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.

Sign up for GitHub

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
+4 −2
Open

Fix bitters cli with custom path #191

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78f9ca8
Allow `bitters reset` and `remove` to take `--path` option
Aug 16, 2015
3e1600e
Minor fix for the shown installation path when running `bitters install`
Aug 16, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/bitters/generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}/"
Copy link

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.

Copy link

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

Copy link

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

bitters/lib/bitters/generator.rb

Line 56 in 8e00402

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.

Copy link

Choose a reason for hiding this comment

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

Makes sense

end
end

desc 'reset', 'Reset Bitters'
method_options path: :string
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used in the code? Can you add tests?

Copy link
Author

Choose a reason for hiding this comment

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

This line isn't used by Bitters itself but would be called if a user runs bitters reset --path foo.
Before this commit, one could install Bitters to a custom path running bitters install --path foo, which makes bitters reset or bitters remove unusable as they weren't accepting a custom path and assuming the default one base/.

remove and reset methods, ends up calling install_path, which returns a path that contains the custom part if --path option is passed, + /base.

I can add some specs for the whole generator code if you think it's necessary. I'll do it then for Bourbon and Neat too (if thoughtbot/neat#329 got merged).

def reset
if bitters_files_already_exist?
remove_bitters_directory
Expand All @@ -32,6 +33,7 @@ def reset
end

desc 'remove', 'Remove Bitters'
method_options path: :string
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used in the code? Can you add tests?

def remove
if bitters_files_already_exist?
remove_bitters_directory
Expand Down Expand Up @@ -62,7 +64,7 @@ def install_files
end

def remove_bitters_directory
FileUtils.rm_rf("base")
FileUtils.rm_rf(install_path)
end

def all_stylesheets
Expand Down