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

feat: interpret demand() numbers as relative to executing command #582

Merged
merged 1 commit into from Aug 9, 2016

Conversation

Projects
None yet
3 participants
@nexdrew
Member

nexdrew commented Aug 9, 2016

Fixes #451.

Previously, min and max values passed to .demand() were interpreted as the total number of non-option arguments, which included the arguments representing the command(s) itself.

With this PR, the min and max values are now interpreted as the additional number of non-option arguments, relative to the currently executing command.

Here's an example:

require('yargs')
  .command(
    'lint',
    'Lint a single file',
    (yargs) => yargs.demand(1, 1),
    (argv) => console.log('linting file:', argv._[1])
  )
  .argv

Before this PR, the command would not work as expected (i.e. would not accept any flagless args) because the lint command itself would satisfy the min and max on the command line.

$ program lint
linting file: undefined

$ program lint example.js
program lint

Too many non-option arguments: got 2, maximum of 1

$ program lint one.js two.js
program lint

Too many non-option arguments: got 3, maximum of 1

After this PR, the command works by requiring a single flagless arg on the command line besides the lint command itself.

$ program lint
program lint

Not enough non-option arguments: got 0, need at least 1

$ program lint example.js
linting file: example.js

$ program lint one.js two.js
program lint

Too many non-option arguments: got 2, maximum of 1

When .demand() is used outside of a command builder, the functionality remains the same.

Note that this is a BREAKING CHANGE since users relying on the previous (now deemed incorrect) functionality will need to change the values they pass to .demand() within command builders, decrementing them by the "nested/depth count" of the command.

Also note that this does not change the contents of argv._ within the command handler; argv._ still contains the command key(s) it took to execute the command(s). (Note the use of argv._[1] in the above example.)

We could possibly change this (which would be another breaking change), but I'd rather leave it this way to potentially support command handlers that are reusable across multiple commands (allowing the handler to slightly change its functionality based on the command keys present in argv._).

@nexdrew nexdrew added the 5.x label Aug 9, 2016

@maxrimue maxrimue referenced this pull request Aug 9, 2016

Closed

Version 5.0.0 #519

10 of 10 tasks complete

@bcoe bcoe merged commit 927810c into master Aug 9, 2016

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@bcoe bcoe deleted the contextual-demand branch Aug 9, 2016

This was referenced Aug 14, 2016

@mathieumg

This comment has been minimized.

Show comment
Hide comment
@mathieumg

mathieumg Oct 17, 2016

Contributor

Just got bit by this not being marked as a breaking change in the v5 changelog, must have slipped through the cracks! 😆

Contributor

mathieumg commented Oct 17, 2016

Just got bit by this not being marked as a breaking change in the v5 changelog, must have slipped through the cracks! 😆

@nexdrew

This comment has been minimized.

Show comment
Hide comment
@nexdrew

nexdrew Oct 17, 2016

Member

@mathieumg Whoops, you're right, looks like we forgot to use the BREAKING CHANGE marker in the merged commit, sorry about that!

Member

nexdrew commented Oct 17, 2016

@mathieumg Whoops, you're right, looks like we forgot to use the BREAKING CHANGE marker in the merged commit, sorry about that!

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe Oct 17, 2016

Member

@nexdrew @mathieumg mind updating the CHANGELOG for v5 post-hoc, save others :)

Member

bcoe commented Oct 17, 2016

@nexdrew @mathieumg mind updating the CHANGELOG for v5 post-hoc, save others :)

@mathieumg

This comment has been minimized.

Show comment
Hide comment
@mathieumg

mathieumg Oct 18, 2016

Contributor

@bcoe Done!

Contributor

mathieumg commented Oct 18, 2016

@bcoe Done!

nexdrew added a commit that referenced this pull request Oct 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment