Skip to content

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

Closed
shadowspawn wants to merge 2 commits intotj:developfrom
shadowspawn:feaure/386-args
Closed

WIP: Keep command out of program.args and add unknown to .action params#1030
shadowspawn wants to merge 2 commits intotj:developfrom
shadowspawn:feaure/386-args

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.

1 participant