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: address ambiguity between nargs of 1 and requiresArg #1572

Merged
merged 2 commits into from Mar 7, 2020

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Mar 2, 2020

In #1054, requiresArg was refactored to use a nargs of 1 to specify that a value was required following an option (--x foo is valid, --x is not valid).

The problem with this design decision was that it created an ambiguity when array and nargs were used in conjunction, one could not distinguish between:

  • an array that should only accept one value.
  • an an array that required at least one value.

This behavior lead to issues like #1570, #1098, and mochajs/mocha#4196.


To solve this problem nargs now accepts the special value NaN, this value is used by requiresArg to specify that an array accepts "one or more" values following it. A nargs of 1 used in conjunction with array, continues to indicate that just one value should be accepted by an array.

@bcoe
Copy link
Member Author

bcoe commented Mar 2, 2020

@kevinoid please try npm i yargs@next, I believe I've come up with a way to support both types of behavior that folks wanted (by changing the behavior of requiresArg slightly).

@kevinoid
Copy link

kevinoid commented Mar 2, 2020

Thanks @bcoe! Unfortunately it didn't appear to fix the example from #1570 for me. Steps I ran:

cd "$(mktemp -d)"
npm init -y
npm install yargs@next
cat > example.js <<'EXAMPLE_JS'
#!/usr/bin/env node
const yargs = require('yargs')
console.log(
  yargs
    .option('header', {
      requiresArg: true,
      array: true,
      nargs: 1
    })
    .argv
)
EXAMPLE_JS
node example.js --header foo bar

Which printed

{ _: [], header: [ 'foo', 'bar' ], '$0': 'example.js' }

which matches the behavior from yargs@15.2.0 rather than yargs@15.1.0.

Let me know if there's anything else I can do to help.

Copy link
Member

@mleguen mleguen left a comment

Choose a reason for hiding this comment

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

I still think that adding min/max parameters to nargs would have been a more readable way of doing this (NaN meaning "1 or more" does not speak much to me), however I agree that this trick does the job, and leads to far less changes in both yargs and yargs-parser than what I was advocating for.

@bcoe bcoe merged commit a5edc32 into master Mar 7, 2020
@bcoe bcoe deleted the array-nargs-explicit branch March 7, 2020 20:34
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.

None yet

3 participants