-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
Enhance rake ember:install
to fully reinstall if necessary.
#396
Conversation
|
||
path_set = build_path_set(app: app) | ||
|
||
expect(path_set.bower_deps).to eq rails_root.join("foo", "bower_components") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [82/80]
@seanpdoyle Ready for review. |
end | ||
|
||
exec "#{paths.npm} prune && #{paths.npm} install" | ||
exec "#{paths.bower} prune && #{paths.bower} install" | ||
clean_ember_dependencies! if invalid_ember_dependencies? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From thoughtbot's Ruby style guide:
Avoid conditional modifiers (lines that end with conditionals).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, but... its so dang readable! :( I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, but... its so dang readable!
We like to refer to them as "surprise conditionals"
-- they make lines so "readable" (in an English sense) that they tend to blend in too well.
Conditionals (and if
/ else
blocks) contain logic that depends on program state. They should jump out at developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good points. I can understand that reasoning.
Ember CLI ships with ember-cli-dependency-checker, which provides the `ember version` command, which is akin to `bundle check`. This commit enhances the ember:install rake task to `rm -rf node_modules bower_components` before installing if `ember version` returns non-zero. This (helped further by npm shrinkwrap) works to ensure that every machine has the same dependencies. Closes [thoughtbot#394]. [thoughtbot#394]: thoughtbot#394
cb5dddc
to
b6e12df
Compare
@seanpdoyle Rebased on master to resolve conflict in CHANGELOG.md. What do you think? |
Merged in 6eceea8. Thanks again Micah! |
🎉 |
Ember CLI ships with ember-cli-dependency-checker, which provides the
ember version
command, which is akin tobundle check
. This commit enhancesthe ember:install rake task to
rm -rf node_modules bower_components
beforeinstalling if
ember version
returns non-zero. This (helped further by npmshrinkwrap) works to ensure that every machine has the same dependencies.
Closes #394.