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

WIP: Keep command out of program.args and add unknown to .action params #1030

Closed
wants to merge 2 commits into from

Conversation

shadowspawn
Copy link
Collaborator

Pull Request

I have not written any new tests or updated docs yet. This is a draft PR to show current code.

Problem

There are problems with the processing of arguments to call .action:

  • extra arguments are ignored and the first one is overwritten
  • the passed array of arguments is mutated, but that ultimately affects program.args, including especially inappropriately adding the command object into that array
  • the combination of issues makes it hard for client code to detect extra arguments

See: #386 #749 #1000

var program = require('commander');

  program
    .option('-v, --verbose', 'Enable verbose output')
    .usage('[options] [command]')

  program
    .command('sub <something>')
    .action(function(oneArgument, cmd, extraArguments) {
      console.log(`Expected argument is: ${oneArgument}`);
      console.log('Extra arguments: ', extraArguments);
      console.log('program.args: ', program.args);
    });

program.parse(["node", "some-file.js", "-v", "sub", 1, 2, 3, 4, 5, 6, 7, 8, 9]);
$ node index.js 
Expected argument is: 1
Extra arguments:  3
program.args:  [ 1,
  Command {
    ... dump of command object!
  3,
  4,
  5,
  6,
  7,
  8,
  9 ]

Solution

Changed code to prepare parameters for .action in a separate array without changing passed array. Pass unexpected parameters to .action as an array rather than just tagged on end.

$ node index.js 
Expected argument is: 1
Extra arguments:  [ 2, 3, 4, 5, 6, 7, 8, 9 ]
program.args:  [ 1, 2, 3, 4, 5, 6, 7, 8, 9 ]

@shadowspawn
Copy link
Collaborator Author

Closing in favour of #1048 where reworked changes after move to Jest.

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

1 participant