Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor #110

Merged
merged 6 commits into from
Apr 22, 2014
Merged

Refactor #110

merged 6 commits into from
Apr 22, 2014

Conversation

nickserv
Copy link
Collaborator

  • Homesick is now a module, not a class.
  • The original Thor-related code in Homesick is now in the Homesick::CLI class.
  • autoload calls have been removed, since it was an easy fix after ensuring that Homesick was a module (fixes Don't use autoload #108).
  • Actions has now been split up into two modules: Actions::FileActions and Actions::GitActions.

@JCook21
Copy link
Collaborator

JCook21 commented Apr 17, 2014

👍 Looks good and tests are still passing.

@nickserv
Copy link
Collaborator Author

I also tried splitting up CLI into two modules, one for normal commands and one for git commands (especially since those might be deprecated or aliased eventually). However, I though it would probably be tricky with what's going on with Thor, and possibly confusing.

I'm thinking it might just be best to move the git commands to the bottom of the file together. What do you think?

@JCook21
Copy link
Collaborator

JCook21 commented Apr 20, 2014

I'm not too worried about moving the Git commands, although feel free to do that if you like. I would like to get this merged though. Since there are quite a few changes in this I'd feel happier if someone else reviewed this first though. What do you think?

@nickserv
Copy link
Collaborator Author

Since there are quite a few changes in this I'd feel happier if someone else reviewed this first though.

I agree. @technicalpickles would you mind checking this? My switch from autoload to require seems stable to me, but I just want to check that I haven't accidentally changed something unintentionally (since it's not loading those modules lazily any more).

@technicalpickles
Copy link
Owner

Looks good overall to me. Having a specific class for CLI is a pretty common pattern, and agree on Actions starting to get git. How do the tests look? Have you had much luck running this locally against your dotfiles? If you are happy with both those, should be good to merge & release.

@nickserv
Copy link
Collaborator Author

Build 194 for b3bd7a1 is passing.

rake install works fine for me, and so do most other commands after some manual testing. I just noticed an issue with homesick symlink which is also in master, so I'll report that over at #112.

I would say that this is ready to merge, though we should probably wait to release until we figure out what's going on with this new bug.

@JCook21
Copy link
Collaborator

JCook21 commented Apr 22, 2014

Alright, seeing as this is looking good I'm going to merge it.

JCook21 pushed a commit that referenced this pull request Apr 22, 2014
@JCook21 JCook21 merged commit b5138bc into technicalpickles:master Apr 22, 2014
@nickserv nickserv deleted the refactor branch April 22, 2014 16:53
@nickserv nickserv mentioned this pull request Apr 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't use autoload
3 participants