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: support multiple values after --ignore-files again #1652

Merged
merged 1 commit into from
Jul 12, 2019

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Jul 8, 2019

The yargs upgrade from 6.6.0 to 13.2.1 in edebef4 broke the documented ability to pass multiple files to ignore after the --ignore-files (aka -i) parameter. This is because yargs 11 started to treat requiresArg as an implicit nargs: 1, i.e. to accept exactly one value only.

There is an open upstream bug about this issue: yargs/yargs#1098
This patch works around the issue by dropping requiresArg and validating the number of parameters in the execute function instead.

Fixes #1641

The yargs upgrade from 6.6.0 to 13.2.1 in edebef4 broke the
documented ability to pass multiple files to ignore after the
`--ignore-files` (aka `-i`) parameter.

This is because yargs 11 started to treat `requiresArg` as an implicit
`nargs: 1`, i.e. to accept exactly one value only.

This patch fixes the issue by dropping `requiresArg` and validating the
number of parameters in the `execute` function instead.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4c60c81 on Rob--W:unbreak-multiple-ignore-files-params into 72bb28c on mozilla:master.

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this issue @Rob--W
The fix for the --ignore-files option looks good to me.

While I was looking into the changes in this PR I also noticed that:

  • it looks that --pref and --start-url are two other cli options that have changed behavior for the same underlying reason, but it seems that in the MDN docs we never suggested the syntax that has been regressed (it may still be reasonable to keep the behavior consistent across all the "array type" cli options, but not really necessary for proceeding with this PR, we can evaluate that in a follow up issue and/or PR)

  • I just noticed that we never included requiresArgs: true in the definition of the --arg cli option, and so it is currently considered valid even if it being used without any parameter (as in web-ext run -a), it is not a super big deal, but still something that we should fix (this is also reasonable to be deferred to a separate PR)

@Rob--W
Copy link
Member Author

Rob--W commented Jul 12, 2019

Thanks for digging into this issue @Rob--W
The fix for the --ignore-files option looks good to me.

While I was looking into the changes in this PR I also noticed that:

  • it looks that --pref and --start-url are two other cli options that have changed behavior for the same underlying reason, but it seems that in the MDN docs we never suggested the syntax that has been regressed (it may still be reasonable to keep the behavior consistent across all the "array type" cli options, but not really necessary for proceeding with this PR, we can evaluate that in a follow up issue and/or PR)

The old behavior was not documented, and nobody complained about it, so I guess that we're good to go with supporting one value per flag. I have no strong opinions either way, though based on the fact that it is not possible to easily support requireArgs: true, with multiple values, I am more favorable to keeping the current behavior.

  • I just noticed that we never included requiresArgs: true in the definition of the --arg cli option, and so it is currently considered valid even if it being used without any parameter (as in web-ext run -a), it is not a super big deal, but still something that we should fix (this is also reasonable to be deferred to a separate PR)

Requiring a parameter for -a would be a backwards-incompatible change, and require a major version bump. Definitely something outside the scope of this patch, which aims to fix a regression from 3.0.0 -> 3.1.0.

@rpl
Copy link
Member

rpl commented Jul 12, 2019

@Rob--W I agree, I mentioned both because I noticed them while looking to this change but, as I said, I don't consider neither of them as blocking.

@rpl rpl merged commit ff0ffa5 into mozilla:master Jul 12, 2019
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.

sign with --ignore-files argument causes "This command does not take any arguments" error
3 participants