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

feat: apply default builder to command() and apply fail() handlers globally #583

Merged
merged 2 commits into from Aug 12, 2016

Conversation

@nexdrew
Copy link
Member

@nexdrew nexdrew commented Aug 9, 2016

Addresses this TODO from #545:

TODO: default to a noop builder in yargs@5.x

When implementing this, I discovered it was necessary to either (a) change the expectations of some tests to move a top-level .fail() handler to a command-specific one or (b) start applying .fail() handlers globally. I opted for the latter due to conversation in #539.

Therefore, this PR solves 2 main problems:

  1. Do not require a command to specify a builder in order for validation to apply

    Prior to this, code such as .command('hi').demand('name').global('name') would not enforce the globally required --name option when hi is given on the command line, which has been outlined here, because "running" a command without a builder would actually bypass a reset, reparse, and subsequent validation.

    This PR changes that so a command like .command('hi') is now equivalent to a command like .command('hi', '', {}).

    Note that I have an alternative implementation that applies validation to builder-less commands without the reset or reparse (and means options do not have to be global to be, e.g., required), but I prefer the approach of this PR since it (i) causes all commands to be treated equally as running within a "command context" and (ii) satisfies the previous TODO (and whatever thought process/concerns went with it).

    Accepting this PR means options must be explicitly global (or command-specific) in order to apply to a command.

  2. Allow a top-level .fail() handler to handle errors that occur during command execution

    As previously discussed in #539.

    This also means that multiple fail handlers at different "command depths" will all be called in depth-first, reverse order. For example, the following code:

    require('yargs')
      .fail((msg, err) => {
        console.log('top-level fail handler 1')
      })
      .command(
        'hi', '',
        (yargs) => {
          yargs
            .fail((msg, err) => console.log('command fail handler 1'))
            .fail((msg, err) => console.log('command fail handler 2'))
        }
      )
      .fail((msg, err) => {
        console.log('top-level fail handler 2')
      })
      .demand('name').global('name').argv

    Would produce this:

    $ node validate-command.js hi
    command fail handler 2
    command fail handler 1
    top-level fail handler 2
    top-level fail handler 1

Note that both of these are BREAKING CHANGES.

nexdrew added 2 commits Aug 9, 2016
in order to apply reset, reparse, and validation to all commands
@nexdrew nexdrew added the 5.x label Aug 9, 2016
@maxrimue maxrimue mentioned this pull request Aug 9, 2016
10 of 10 tasks complete
@bcoe bcoe merged commit 0aaa68b into master Aug 12, 2016
5 checks passed
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 First build on validate-commands-option2 at 100.0%
Details
@bcoe bcoe deleted the validate-commands-option2 branch Aug 12, 2016
@kyeotic kyeotic mentioned this pull request Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.