allowUnknownOption handling in .parseOptions()

[aweebit]

The docs says:

By default the option processing shows an error for an unknown option. To have an unknown option treated as an ordinary command-argument and continue looking for options, use .allowUnknownOption().

However, unknown options are not treated as command-arguments by .parseOptions() when allowUnknownOption is used. That can be shown by a simple example:

const program = new Command().allowUnknownOption();
console.log(program.parseOptions(['arg'])); // { operands: [ 'arg' ], unknown: [] }
console.log(program.parseOptions(['--option'])); // { operands: [], unknown: [ '--option' ] }

This might seem harmless, but actually has consequences. The following test from tests/command.helpCommand.test.js, line 24 would have to fail if unknown options were treated as described in the docs since caughtErr.code would equal to 'commander.unknownCommand' instead of 'commander.help'.

test('when program has subcommands and specify only unknown option then display help', () => {
  const program = new commander.Command();
  program
    .configureHelp({ formatHelp: () => '' })
    .exitOverride()
    .allowUnknownOption()
    .command('foo');
  let caughtErr;
  try {
    program.parse(['--unknown'], { from: 'user' });
  } catch (err) {
    caughtErr = err;
  }
  expect(caughtErr.code).toEqual('commander.help');
});

Some possible solutions I see:

• treat unknown options as command-arguments in .parseOptions() as suggested by the docs and
  • either remove / change the test accordingly
  • or make .unknownCommand() treat options specially
• remove the part about treatment as a command-argument from the docs so that it is not misleading anymore

[aweebit]

treat unknown options as command-arguments in .parseOptions() as suggested by the docs and

• either remove / change the test accordingly

I opened #1946 with an implementation of this suggestion.

[shadowspawn]

.parseOptions() is a low level routine that only processes the arguments using the context of the current command.

.allowUnknownOption is not relevant to .parseOptions(). The comment quoted from the README is about the .parse() outcomes.

In .parseOptions() the input arguments are either:

• a recognised option and consumed, including option-argument if appropriate
• something which is not an option or option-argument, aka operand
• unknown so far, more processing required

The arguments across those last two categories are kept in order. In internal use, they get passed into the subcommand for further processing.

import { program} from 'commander';

program.option('--global');

program.command('sub')
   .option('--known');

const exampleArgs = ['--global', 'sub', '--known', '--unknown', 'sub-arg'];
console.log(program.parseOptions(exampleArgs));

 % node parseOptions.mjs 
{ operands: [ 'sub' ], unknown: [ '--known', '--unknown', 'sub-arg' ] }

[aweebit]

.parseOptions() is a low level routine that only processes the arguments using the context of the current command.

.allowUnknownOption is not relevant to .parseOptions().

In #1944 (comment), you said:

I think allowUnknownOption is more likely to apply to particular commands than all commands. So somewhat like .passThroughOptions(), for special cases.

But despite allowUnknownOption and passThroughOptions being similar, passThroughOptions is not irrelevant to .parseOptions() like you now say allowUnknownOption is.

The comment quoted from the README is about the .parse() outcomes.

The .parse() outcome is affected by this check not being made as the aforementioned test demonstrates (it fails at 5e35531, the first commit of #1946, in which the test was not removed yet). Also, listeners attached to the legacy command: events currently receive operands and unknown values that contradict what the docs suggests, which might not be important, but in case there are more places where operands and unknown are used in the future, we sure would like the values to be correct.

The arguments across those last two categories are kept in order.

They still are in #1946, it's just that the switch from operands to unknown is not made when an unknown option is encountered and allowUnknownOption is on, unless a subcommand has already been reached.

In internal use, they get passed into the subcommand for further processing.

They still do in #1946 (see the last part of the previous sentence). !reprocessedBySubcommand is included in the check exactly for that reason: so that unknown options that come after entering a subcommand can be reprocessed.

[shadowspawn]

I basically don't understand what your real concern is. I'll add a couple more explanations.

---

The docs says:

By default the option processing shows an error for an unknown option. To have an unknown option treated as an ordinary command-argument and continue looking for options, use .allowUnknownOption().

The point of the comment is that people might think the unknown options get auto-detected or put somewhere else. They just get returned in program.args along with the ordinary command-arguments.

import { program} from 'commander';

program
  .allowUnknownOption()
  .option('--known');

program.parse();

console.log(`opts: %o`, program.opts());
console.log(`args: %o`, program.args);

% node index.mjs --unknown --known foo
opts: { known: true }
args: [ '--unknown', 'foo', [length]: 2 ]

Do you have a suggestion for a better wording?

---

This might seem harmless, but actually has consequences. The following test from tests/command.helpCommand.test.js, line 24 would have to fail if unknown options were treated as described in the docs since caughtErr.code would equal to 'commander.unknownCommand' instead of 'commander.help'.

My recollection is I wrote that test to capture a specific error case I encountered where I felt the default behaviour would not give the best information to the end-user and I customised the outcome (and wrote a test because it was on purpose).

[aweebit]

This might seem harmless, but actually has consequences. The following test from tests/command.helpCommand.test.js, line 24 would have to fail if unknown options were treated as described in the docs since caughtErr.code would equal to 'commander.unknownCommand' instead of 'commander.help'.

My recollection is I wrote that test to capture a specific error case I encountered where I felt the default behaviour would not give the best information to the end-user and I customised the outcome (and wrote a test because it was on purpose).

I agree the commander.unknownCommand error would be very confusing in this case. I have now made the change necessary to revert the test removal in the branch I originally created for the now-closed #1946, see 567e0b6...026b36b. (Please also have a look at 7dcbbea giving the test a name that is more precise and moving it to a better place.)

It looks like there aren't any visible changes left (at least if we ignore the fact that .parseOptions() is a public method). However, I still believe unknown is not the right place for an allowed unknown option that comes before a subcommand (and not the right place for all the following arguments, too). Here is the most extensive reasoning I could come up with:

• As already mentioned, .parseOption() belongs to the public API and should therefore produce reasonable results
• The command: event listeners should receive reasonable operands and unknown values despite being a legacy feature (the change I suggest is a bug fix, not just an out-of-the-blue change in behavior)
• Even if we ignore the previous two reasons, there is still a chance the values of operands and unknown returned from .parseOptions() will be used in new places in the library code in the future, and if they are, we want the values to be reasonable from the start so that contributors don't have to spend hours looking for the cause of inconsistencies and odd behavior that would then just turn out to be the fact that allowed unknown options are not handled properly, and so the change I suggest here would still have to be applied
• Putting an unknown option that comes before a subcommand as well as all the arguments following it in operands rather than in unknown is reasonable because
  • that is what the fact unknown options are to be treated as ordinary command-arguments mentioned in the docs implies
  • it is logical for the fact that the first element of unknown is an option flag to indicate that a badly formatted input had been provided (unless the array can be passed on to a subcommand), which is not the case when allowUnknownOption is on
  • allowUnknownOption behavior should be consistent with how its brother passThroughOptions behaves, and its behavior includes putting unknown options that it tolerates/allows (those coming after a command-argument) in operands, not in unknown
  • that would make return values of .parseOptions() consistent when allowUnknownOption and passThroughOptions are combined (currently, all unknown options coming after known options are put in operands as mentioned above, but only if the first argument that is not a known option is not an unknown option but an actual, ordinary command-argument, otherwise they are all put in unknown which makes zero sense)

A late addition: You also said this in a comment above:

In .parseOptions() the input arguments are either:

• <…>
• unknown so far, more processing required

But there is no processing required for unknown options found before a subcommand when using allowUnknownOption! So even according to the semantics you wrote out yourself, unknown is not the right place for those options.

---

I realize this is a very small and subtle technical detail, so the change is hard to “sell”, but I hope I still managed to convince you it is worth it. If so, please consider reopening #1946 (click here to see all the implied file changes). If not, feel free to ask for further clarification or close the issue.

[aweebit]

I have just published feature/parseOptions-rework combining the changes from #1934 and #1946 into one big .parseOptions() rework. You are welcome to have a look at the diff.

[shadowspawn]

I had a reread of #1946, and looked at the combo branch, and reread the extensive reasoning multiple times.

Not convinced, sorry.

If not, feel free to ask for further clarification or close the issue.

Closing the issue!