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: make requiresArg work with arrays #136

Merged
merged 1 commit into from
Oct 6, 2018
Merged

Conversation

vmx
Copy link
Contributor

@vmx vmx commented Sep 28, 2018

When requiresArg was used together with array, only the first
argument of the array would end up being parsed. With this change
things work as expected.

The requireArg option sets the nargs options to 1, so only
one additional argument will be parsed. The problem was that that
parsing step was checked before the array parsing one. The solution
is as simple as flipping those if conditions and check if its an
array first, hence ignoring the nargs option.

Fixes yargs/yargs#1170.

When `requiresArg` was used together with `array`, only the first
argument of the array would end up being parsed. With this change
things work as expected.

The `requireArg` option sets the `nargs` options to `1`, so only
one additional argument will be parsed. The problem was that that
parsing step was checked before the array parsing one. The solution
is as simple as flipping those if conditions and check if its an
array first, hence ignoring the `nargs` option.

Fixes yargs/yargs#1170.
@bcoe
Copy link
Member

bcoe commented Oct 6, 2018

@vmx thank you for the contribution, I just restarted the build.

@bcoe bcoe merged commit 77ae1d4 into yargs:master Oct 6, 2018
@bcoe
Copy link
Member

bcoe commented Oct 6, 2018

@vmx please try npm i yargs@next which should have this work landed \o/ if things are looking good, I'll land a new version of yargs early in the week.

@vmx vmx deleted the array-required-arg branch October 8, 2018 10:05
@vmx
Copy link
Contributor Author

vmx commented Oct 8, 2018

@bcoe I can confirm that with yargs@next (yargs@12.0.3-candidate.0) things now work as expected.

@boneskull
Copy link
Contributor

This change means I can't constrain the number of values read in an array-type option.

IMO, this should be reverted until a better solution arises, because I cannot find a workaround. Before this change, check() could have been used for validation.

boneskull added a commit to boneskull/yargs-parser that referenced this pull request Nov 19, 2018
@bcoe
Copy link
Member

bcoe commented Nov 19, 2018

@vmx, @boneskull and I just had a discussion around this here:

http://devtoolscommunity.herokuapp.com/

Because this is a breaking change, we're going to temporarily roll back for @boneskull's usage with mocha, but here's what I'd like to do:

  1. we introduce the greedy-arrays option, which will default to false in yargs-parser; this will indicate that array array will parse a sequence of values like -x 1 2 3 (behavior that can break commands, so should be opted into).
  2. we'll re-and your work here; but make it so takes into account greedy-arrays.

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.

3 participants