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 --skip-foo switches when there's a trailing non-switch #679

Merged
merged 1 commit into from
Sep 24, 2019
Merged

Fix --skip-foo switches when there's a trailing non-switch #679

merged 1 commit into from
Sep 24, 2019

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Sep 23, 2019

I was running the command bundle exec rails new --dev --skip-webpack-install my_project inside my Rails clone, but Rails generators kept running rails webpacker:install.

Turns out thor was considering my_project as the value for the --skip-webpack-install switch. However, I think the command line I specified is correct and non-ambiguous and that --no- and --skip- flags are not supposed to have a value.

This PR fixes this edge case.

It adds two tests, the first one is already passing against master, the second one is fixed by this PR. So, I think it also makes the behaviour more consistent.

Fixes #580.

@deivid-rodriguez
Copy link
Contributor Author

I suspected that more people would've run into this and I found an open issue: #580. I'll edit the PR description so it gets autoclosed if/when merged.

@rafaelfranca rafaelfranca merged commit b76f2c0 into rails:master Sep 24, 2019
@deivid-rodriguez deivid-rodriguez deleted the fix_skip_args_issue branch September 25, 2019 06:18
@doudou
Copy link
Contributor

doudou commented Sep 25, 2019

Thanks for fixing this ... I completely forgot about it, got used to always specify the boolean flags with =f instead (--webpack-install=f).

@deivid-rodriguez
Copy link
Contributor Author

No problem!

@deivid-rodriguez
Copy link
Contributor Author

@doudou Funny enough, my fix broke your workaround and I got beaten by it myself. I added a follow up PR to restore support for that syntax so that you and I don't have to change all of our scripts, tests or whatever to use the previous syntax.

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.

boolean options changed in v0.20.0
3 participants