passThroughOptions being a superset of enablePositionalOptions

[aweebit]

new Command()
  .passThroughOptions()
  .command('sub')
  .passThroughOptions();

Running this code results in the following error:

Error: passThroughOptions can not be used without turning on enablePositionalOptions for 
parent command(s)
    at Command.passThroughOptions (lib/command.js:739:13)

But actually, using passThroughOptions on a subcommand whose parent also uses it should be okay because passThroughOptions makes options positional just like enablePositionalOptions. That is demonstrated by the following snippet

const program = new Command()
  .passThroughOptions()
  .option('-x');
const sub = program.command('sub')
  .option('-x');
program.parse(['sub', '-x'], { from: 'user' });
console.log(program.opts()); // {}
console.log(sub.opts()); // { x: true }

and explained by the fact passThroughOptions is included in the check in lib/command.js, line 1522.

So is there any point in requiring enablePositionalOptions when passThroughOptions which too has the needed effect is already on?

My suggestion is

• either not to throw the error when the parent command uses passThroughOptions,
• or to change the semantics for passThroughOptions so that there is no functionality overlap with enablePositionalOptions.

The latter idea introduces a breaking change but could be justified by some absurd use cases like

# --verbose to be passed through due to using passThroughOptions
node program --trace curl --verbose https://github.com/

# --verbose to be consumed by program command due to not using enablePositionalOptions
node program utility-subcommand --verbose

[shadowspawn]

In normal use, .enablePositionalOptions() will be enabled globally while .passThroughOptions() will be enabled in specific leaf commands.

The two concepts are separate.

[aweebit]

The two concepts are separate.

I see, but there is still a functionality overlap that makes the error unnecessary when the parent command uses passThroughOptions.

In normal use, .enablePositionalOptions() will be enabled globally while .passThroughOptions() will be enabled in specific leaf commands.

Not only in leaf commands, but also in commands that have both subcommands and an action handler attached, so the use in parent commands is justified. And when it is used in a parent command, it additionally has the effect of enablePositionalOptions, which is why the error becomes unnecessary.

[shadowspawn]

I understand the error may not be strictly required. I'll take another look, but it is currently as I intended.

[aweebit]

Here is my mental model for how options should be handled ideally given different settings in which there is no functional overlap between settings, and including the modified semantics for operands and unknown suggested in the now-closed #1945 in italics.

The change suggested by this issue is in bold.

Default behavior:

Consume all known options.

Pass unknown options on for reprocessing by pushing to unknown in .parseOptions() if there is a subcommand.
Otherwise, exit with an error message as soon as an unknown option is found.

When using enablePositionalOptions:

Same as default, but don't consume known options after subcommand.

Since it is now not needed, don't process arguments after subcommand in .parseOptions() and instead pass them to the subcommand right away by pushing to unknown. Currently also happens when using passThroughOptions, but should not!

When using passThroughOptions:

Same as default, but if there is no subcommand and a command-argument is encountered, don't consume the remaining known options and ignore the remaining unknown options. All the remaining options are treated as command-arguments and therefore pushed to operands in .parseOptions() in this case.

When using allowUnknownOption:

Same as default, but if there is no subcommand, ignore the remaining unknown options (treated as command-arguments and therefore pushed to operands in .parseOptions() in this case).

There is no functional overlap…

• …between enablePositionalOptions and the other two: because enablePositionalOptions only affects the behavior when there is a subcommand, but the other two only affect it when there is no subcommand (new for passThroughOptions).

• …between passThroughOptions and allowUnknownOption:

  • because if only passThroughOptions is used, unknown options before command-arguments still produce an error, but that does not happen when allowUnknownOption is used;
  • and because if only allowUnknownOption is used, known options are still consumed after a command-argument has been found, but that does not happen when passThroughOptions is used;

  so only using one of the settings has a different effect than using both of them. (That is different from how using both passThroughOptions and enablePositionalOptions is redundant in the current implementation because just using passThroughOptions has the same effect.)

P.S.

Reiterating the suggestion from #1945

As seen here, the suggestion from #1945 would make unknown only ever used to store arguments that are passed on to a subcommand for further processing (even your comment #1945 (comment) suggested that should be the case).

If I still wanted to document the operands and unknown semantics here without the suggested modification applied, it would get really, really messy, especially because the combination of passThroughOptions and allowUnknownOption is currently confusing as shown in #1945 (comment), and so the passThroughOptions section would have to include a remark on the special treatment of the first argument when it is an unknown option and the combo is used.

Both this comment and #1936 (comment) show that the suggestion from #1945 has the potential to make some explanations about the library's internals less confusing and more precise.

[shadowspawn]

I understand the error may not be strictly required. I'll take another look, but it is currently as I intended.

I am still happy with the behaviour.

Morally, perhaps the code could check this._passThroughOptions more selectively than this._enablePositionalOptions as I think pass-through is only needed for the default command, but easier to just test at the top of the block and not be too clever.

commander.js/lib/command.js

Line 1522 in 4ef19fa

if ((this._enablePositionalOptions || this._passThroughOptions) && operands.length === 0 && unknown.length === 0) {

[shadowspawn]

In absence of other issues coming up, I don't currently want to change the behaviour.

Thank you for your contributions.