On command and subcommand reusability and setting inheritance

[aweebit]

Is it expected behavior that parse() / parseAsync() are only correctly callable once, since the command's properties get polluted with the result of the previous call?

I think it should not be expected. Use cases for repeated calls include testing and interactive command parsing.

Here is an example:

program
  .option('-a')
  .option('-b');

program.parse(['-a'], { from: 'user' });
program.parse(['-b'], { from: 'user' });

console.log(program.opts()); // { a: true, b: true }

If it is not expected, a possible solution is to do a cleanup at the beginning of parse() / parseAsync().

Is it expected behavior that explicitly constructed subcommands are tightly bound to one parent command and cannot be used as stand-alone commands or as subcommands of multiple parent commands?

Here is an example:

const sub = new Command('sub')
  .action(() => {});

program
  .hook('preAction', (_, actionCommand) => {
    console.log('preAction', actionCommand.args[0]);
  })
  .addCommand(sub);

// preAction 1 is logged despite calling sub as stand-alone command
sub.parse(['1'], { from: 'user' });

// preAction 2 is logged as expected
program.parse(['sub', '2'], { from: 'user' });

const anotherParent = new Command()
  .addCommand(sub);

// preAction 3 is not logged since sub has been reused as a subcommand of anotherParent 
program.parse(['sub', '3'], { from: 'user' });

If it is not expected, a possible solution is to not set the parent property on subcommands in command() and addCommand() but in _dispatchSubcommand() instead, and unset it before returning.

If it is expected, an error should be thrown when trying to parse or adopt a command that already has a parent.

Is it expected behavior that implicitly constructed subcommands added with command() only inherit settings upon construction?

Here is an example:

const sub = program.command('sub');
program.allowExcessArguments(false);

console.log(program._allowExcessArguments); // false
console.log(sub._allowExcessArguments); // true

program.parse(['sub', 'abc'], { from: 'user' }); // succeeds

If it is not expected, a possible solution is to not call copyInheritedSettings() on subcommands added with command() upon construction but in _dispatchSubcommand() instead.

If it is expected, a note on this in the docs would be nice.

[shadowspawn]

Thanks for the clear examples. These are all behaviours that catch an occasional person out, but have not featured in enough issues to document more explicitly or code protection against or consider changing.

Is it expected behavior that parse() / parseAsync() are only correctly callable once, since the command's properties get polluted with the result of the previous call?

Yes. Make a new Command for each call to parse, for tests or REPL.

Related: #438 #1565 #1819 #1829

Is it expected behavior that explicitly constructed subcommands are tightly bound to one parent command and cannot be used as stand-alone commands or as subcommands of multiple parent commands?

Yes.

Is it expected behavior that implicitly constructed subcommands added with command() only inherit settings upon construction?

Yes. The intended pattern is set the global properties on the program before adding subcommands.

[aweebit]

I opened #1917 to add warnings about and errors caused by wrong library usage.

Is it expected behavior that parse() / parseAsync() are only correctly callable once, since the command's properties get polluted with the result of the previous call?

Yes. Make a new Command for each call to parse, for tests or REPL.

Related: #438 #1565 #1819 #1829

Is there a specific reason why this is the expected behavior? Are there cases where building upon the result of the previous parse call is a reasonable thing to do? I cannot really think of any, so why not make repeated parse calls intuitive by simply doing the cleanup I suggested? There even seems to have been enough issues for which this change would be relevant.

[aweebit]

If it is not expected, a possible solution is to do a cleanup at the beginning of parse() / parseAsync().

Opened #1919 with an implementation of this suggestion.

[shadowspawn]

Is there a specific reason why this is the expected behavior?

Not a use-case reason, rather an implementation reason. Originally the code was written with a global instance and no consideration of re-use. This caused exciting conflicts when the unit testing library (also using Commander) affected the user units tests, let alone unit tests conflicting with each other. The approach to make a new Command solved both problems in a simple and robust way. Easy to maintain and easy pattern for user to follow.

[shadowspawn]

All of the behaviours are as expected. I will continue to monitor issues for support for updating the documentation or changing the behaviour. Of the three, I think .parse() being a one-shot is the most common question. I wonder whether a small section on unit-testing would be useful, rather than covering it separately. (Issues for a REPL do come up, but I think that is a rare use case.)

Thank you for your contributions.