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

Fix bitters cli with custom path #191

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mehlah
Copy link

@mehlah mehlah commented Aug 16, 2015

Allow bitters reset and bitters remove to take --path option

Mehdi Lahmam added 2 commits August 16, 2015 19:22
In case of a custom path used for Bitters installation, `remove` or
`reset` commands were useless as not aware of this path.
Running `bitters install --path foo` outputs "Bitters files installed to
"foo/base/base" when it should show "foo/base" only.
@@ -16,11 +16,12 @@ def install
puts "Bitters files already installed, doing nothing."
else
install_files
puts "Bitters files installed to #{install_path}/base"
puts "Bitters files installed to #{install_path}/"

Choose a reason for hiding this comment

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

Do not write to stdout. Use Rails' logger if you want to log.

Choose a reason for hiding this comment

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

We're not in rails, so this is fine

Choose a reason for hiding this comment

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

I would remove the trailing slash, if you specify the path to be custom_path/ it will be printed as ...installed to custom_path//

Copy link
Author

Choose a reason for hiding this comment

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

Not really, as the path would be custom_path/base, due to

Pathname.new(options[:path].to_s).join('base')
.
This diff line (from the 2nd commit), only fixes the CLI output when running for example bitters install --path foo. It outputs Bitters files installed to foo/base/base when it should show foo/base.

Choose a reason for hiding this comment

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

Makes sense

@mehlah
Copy link
Author

mehlah commented Sep 29, 2015

Any updates guys? Anything more I can do to help with this?
@gabebw I answered your comment at #191 (comment)
@teoljungberg I answered your comment at #191 (comment)

@teoljungberg
Copy link

@mehlah I agree with @gabebw, we can still add tests without adding Thor to the project. This is just testing the rails generators, which is already possible to do

@whmii
Copy link
Contributor

whmii commented Mar 3, 2016

@teoljungberg any sense on next steps/recommendations?

@teoljungberg
Copy link

I would still like to see tests for thi.s

@tysongach tysongach requested a review from dgalarza June 26, 2017 14:58
@tysongach
Copy link
Contributor

@mehlah Hi! Are you able to add tests to this PR?

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.

6 participants