Skip to content

Add .allowExcessArguments() and error message - #1407

Merged
shadowspawn merged 7 commits into
tj:release/7.xfrom
shadowspawn:feature/error-for-unexpected-arrguments
Dec 4, 2020
Merged

shadowspawn merged 7 commits into
tj:release/7.xfrom
shadowspawn:feature/error-for-unexpected-arrguments

Conversation

@shadowspawn

@shadowspawn shadowspawn commented Dec 2, 2020 •

Copy link
Copy Markdown
Collaborator

Pull Request

Problem

By default, excess arguments are silently ignored. Checking manually is a bit tricky.

See: #259 #749 #1000 #1268

Solution

  • add a check for excess arguments before calling action handler
  • .allowExcessArguments() is like .allowUnknownOption() to enable/disable the new error
  • off by default (for now), but will be on by default for CommandStrict, see WIP: CommandStrict #1404

Like missing arguments, only checked if there is an action handler.

ChangeLog

Later

I have not added to the README yet and will do that separately. (Hopefully in combination with CommandStrict if that works out.)

@shadowspawn
shadowspawn changed the base branch from master to release/7.x December 2, 2020 01:55
@shadowspawn shadowspawn changed the title WIP: add .allowExcessArguments and error message Add .allowExcessArguments and error message Dec 2, 2020
@shadowspawn
shadowspawn marked this pull request as ready for review December 2, 2020 06:06
@shadowspawn shadowspawn added this to the v7.0.0 milestone Dec 2, 2020
@shadowspawn shadowspawn changed the title Add .allowExcessArguments and error message Add .allowExcessArguments() and error message Dec 2, 2020
@shadowspawn

Copy link
Copy Markdown
Collaborator Author

Example

const { program } = require('commander');

program
  .arguments('<file')
  .allowExcessArguments(false)
  .action(() => {
    console.log('Called main action handler')
  });

program
  .command('sub <first> <second>')
  .allowExcessArguments(false)
  .action(() => {
    console.log('Called subcommand action handler')
  });

program.parse();
$ node foo.js alpha beta
error: too many arguments. Expected 1 argument but got 2.
$ node foo.js sub alpha beta
Called subcommand action handler
$ node foo.js sub alpha beta gamma
error: too many arguments for 'sub'. Expected 2 arguments but got 3.

Comment thread index.js Outdated
* @param {Boolean} [allowExcess] - if `true` or omitted, no error will be thrown
* for excess arguments.
*/
allowExcessArguments(allowExcess) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are the default parameters available?

allowExcessArguments(allowExcess = true) 

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good question, I'll look into that. We have a few methods with this sort of pattern that could use it. Likely can because we have moved forward our minimum version of node.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Defaults from node 6, which is our minimum supported version: https://node.green/#ES2015-syntax-default-function-parameters

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added default parameter instead of manual test to multiple routines.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for fixing it.

@abetomo abetomo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great!

@shadowspawn
shadowspawn merged commit 09f277a into tj:release/7.x Dec 4, 2020
@shadowspawn
shadowspawn deleted the feature/error-for-unexpected-arrguments branch December 4, 2020 09:15
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Dec 6, 2020
@shadowspawn shadowspawn mentioned this pull request Jan 3, 2021
3 tasks done
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Jan 16, 2021
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.

2 participants