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

Add .allowExcessArguments() and error message #1407

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Dec 2, 2020

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
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.

index.js Outdated
* @param {Boolean} [allowExcess] - if `true` or omitted, no error will be thrown
* for excess arguments.
*/
allowExcessArguments(allowExcess) {
Copy link
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
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
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
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
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.

Copy link
Collaborator

@abetomo abetomo left a comment

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 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