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

Allow the user to specify a path from the CLI (#77) #209

Closed
wants to merge 4 commits into from

Conversation

champo
Copy link

@champo champo commented Jan 22, 2015

The CLI command now optionally takes a path to use as destination for
the project. If now path is given by the user, the default of the
project name is used.

The CLI command now optionally takes a path to use as destination for
the project. If now path is given by the user, the default of the
project name is used.
@gfontenot
Copy link
Member

Ignore those Hound warnings. I'll check this out in the AM.

On Jan 21, 2015, at 20:51, Juan Pablo Civile notifications@github.com wrote:

The CLI command now optionally takes a path to use as destination for
the project. If now path is given by the user, the default of the
project name is used.

You can view, comment on, or merge this pull request online at:

#209

Commit Summary

Allow the user to specify a path from the CLI (#77)
File Changes

M lib/liftoff/cli.rb (4)
M lib/liftoff/launchpad.rb (4)
Patch Links:

https://github.com/thoughtbot/liftoff/pull/209.patch
https://github.com/thoughtbot/liftoff/pull/209.diff

Reply to this email directly or view it on GitHub.

@@ -18,7 +18,8 @@ def parse_command_line_options

def global_options
OptionParser.new do |opts|
opts.banner = 'usage: liftoff [-v | --version] [-h | --help] [config options]'
opts.banner = 'usage: liftoff [-v | --version] [-h | --help] ' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about breaking this line up just to appease Hound.

@@ -11,7 +11,7 @@ def liftoff(options)
validate_template
fetch_options

file_manager.create_project_dir(@config.project_name) do
file_manager.create_project_dir(path ? path : @config.project_name) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this logic here feels like a smell to me. I think it would be best to add a path attribute to the project configuration, and put this logic there. The implementation would be fairly trivial:

def path
  @path || project_name
end

This would also let us add a test around the logic.

@gfontenot
Copy link
Member

Sorry for flaking on this for so long.

I really like how simple this ended up being. As I noted, I think it would be best to move the path into the project configuration itself. That seems like it would be the cleanest way to handle it.

@gfontenot
Copy link
Member

I need to turn off Hound since they changed their defaults away from our style. Ignore all of those.

@@ -7,7 +7,7 @@ def initialize(argv)

def run
parse_command_line_options
LaunchPad.new.liftoff @options
LaunchPad.new.liftoff @options, @argv.first
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you set this at the very bottom of the option parsing, you'll be able to remove a lot of the logic around adding this to the configuration. Should be as simple as @options[:path] = @argv.first

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reluctant about that. It feels weird because it could mean that you can preset the path from the config file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but most people won't. The path passed on the command line will take precedence anyway, so I don't think it's that big of a deal. I'm more interested in simplifying this code path than preventing a weird customization.

@@ -109,6 +109,28 @@
end
end

describe '#path' do
context 'when the path is not given in the constructor' do
it 'return the project name' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return -> returnS

@gfontenot
Copy link
Member

@champo Thanks for this! I'm going to fix up the tests and take @gabebw's comments into account then I'll squash and merge.

@champo
Copy link
Author

champo commented Feb 9, 2015

@gfontenot Beat you to it. I totally forgot about the test after changing the logic. Sorry about that!

@gfontenot
Copy link
Member

Nice! Reworded the tests just a bit and merged as cfd7bf6

Thanks!

@gfontenot gfontenot closed this Feb 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants