passThroughOptions and unknown option handling

[aweebit]

Is there a particular reason why .parseOptions() stops processing args when an unknown option is encountered in the passThroughOptions mode? That complicates the help option refactor I am currently working on in #1934 that aims to achieve maximum consistency between how the help option and all other options are handled, because help flags end up ignored if they come after an unknown option and there is no subcommand.

Checkout 02fde0e and run the following code for a demo:

const program = require('commander');
program
  .passThroughOptions()
  .allowUnknownOption();
program.parse(['--unknown', '--help'], { from: 'user' }); // no help displayed
console.log(program.args); // [ '--unknown', '--help' ]
program
  .allowUnknownOption(false);
program.parse(['--unknown', '--help'], { from: 'user' }); // expected help, got error: unknown option '--unknown'

I tried to change the behavior to the one implied by the docs and comments in code so that the processing is only stopped when a command-argument is encountered (see fcfbec1), but that resulted in a test failing.

Relevant test from tests/command.positionalOptions.test.js (line 423)

  test('when passThroughOptions and unknown option then arguments from unknown passed through', () => {
    const program = new commander.Command();
    program
      .passThroughOptions()
      .allowUnknownOption()
      .option('--debug');

    program.parse(['--unknown', '--debug'], { from: 'user' });
    expect(program.args).toEqual(['--unknown', '--debug']);
  });

The behavior I would expect: --debug is consumed and so program.opts() equals to { debug: true } while program.args equals to ['--unknown'].

Relevant lines in .parseOptions() code (on current develop branch)

      // If using passThroughOptions, stop processing options at first command-argument.
      if (this._passThroughOptions) {
        dest.push(arg);
        if (args.length > 0) dest.push(...args);
        break;
      }

The condition on line 2 should be this._passThroughOptions && !maybeOption(arg) so that the comment on line 1 describes what happens well and does not miss the fact the processing is also stopped at unknown options (currently missed in all explanations of passThroughOptions).

[shadowspawn]

Unknown options normally generate an error, so this only comes up when also using .allowUnknownOption(). (This is implied by your code example but may not be obvious to a future reader.)

.allowUnknownOption() suppresses the error handling. The argument isn't an option we recognise, and we can not know whether the following argument might be its option-argument or something else. So the argument is treated as a plain non-option argument and the original argument order is preserved. This is a bit like arguments after a -- to stop option processing. (For historical interest, the code did at one point put the unknown options and the following argument aside probably with thought they might be used as sort of an auto-detected option. But they were never made accessible and it led to argument ordering bugs.)

Quoting from https://github.com/tj/commander.js#parsing-configuration

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(). This lets you mix known and unknown options.

So the combination of features means the unknown option is treated as a command-argument, and .passThroughOptions() stops option processing at the first command-argument.

[aweebit]

The argument isn't an option we recognise, and we can not know whether the following argument might be its option-argument or something else.

So the combination of features means the unknown option is treated as a command-argument, and .passThroughOptions() stops option processing at the first command-argument.

Thanks, this made the reasoning clear for me.

Still, the discrepancy between how the setting is implemented and how it is presented in the docs and in the code comments had had me scratching my head for a while when trying to figure out the internals of .parseOptions().
So what do you think about mentioning the behavior in the docs? For example like his:

To only process options that come before the command-arguments (and before any unknown options), use .passThroughOptions().

[aweebit]

#1942 should fix this issue.

[shadowspawn]

I don't consider allowUnknownOption a first class feature so don't think it needs to be widely covered. Most people don't need to wonder how unknown options fit into the picture. But I'll take a look at the PR.

Recent related comment: #1944 (comment)

[aweebit]

The PR is mainly useful for contributors trying to understand .parseOptions(). The addition to the docs is so tiny that I don't think the specificity of it makes the docs an unsuitable place for it.

[aweebit]

Another attempt at a more clear comment for the early exit due to passThroughOptions:

      // If using passThroughOptions and no subcommand has been found,
      // stop processing as soon as recognised options end.
      // All remaining arguments are treated as command-arguments,
      // except when the first one is an unknown option.
      if (this._passThroughOptions) {
        /* ... */

The last two comment lines are optional. What do you think?

[aweebit]

A variant for #1934:

      // If using passThroughOptions and no subcommand has been found,
      // stop consuming all options except help as soon as recognised options end.
      // Only consume the help option if found before command-arguments.
      // Exit loop as early as possible.
      // All remaining arguments are treated as command-arguments,
      // except when the first one is an unknown option.
      if (this._passThroughOptions) {
        onlyConsumeHelpOption = true;
        if (!this._hasHelpOption || !isArgFlag) {
          dest.push(arg, ...args);
          break;
        }
      }

The last two lines are optional here, too.

[shadowspawn]

Edit: this was in reply to:

// If using passThroughOptions and no subcommand has been found,
// stop processing as soon as recognised options end.

First two lines are ok. I would leave out the "and no subcommand has been found".

(I had another go too, but didn't do any better!)

[aweebit]

I would leave out the "and no subcommand has been found".

That would make the "as soon as recognised options end" part sound a little too strong for someone reading the comment before all the code above it, for example someone who was only looking for implementation details of passThroughOptions in the library's code.

Update: The part you'd like to leave out will also be necessary if it is decided to change the semantics for passThroughOptions in order to remove the functionality overlap with enablePositionalOptions, as suggested in #1947. In that case, the parsing will continue if a subcommand is found. If we add the part now, there will be less chance we forget about it later – not really a reason for including the part, but a nice bonus!

[shadowspawn]

Hmm. There is lots of implied context from the code above, and the comment could be interpreted as being checked in the line below (i.e. passThroughOptions is checked but not recognised subcommand) .

I'll have another go at my abandoned attempt. How about:

// If using passThroughOptions, pass through the remaining args (including options) since have found something which is not a recognised option

[aweebit]

There is lots of implied context from the code above

It is all summarized very well in the two lines I suggested, so a lazy reader would not have to go look at the code above to figure it out.

the comment could be interpreted as being checked in the line below (i.e. passThroughOptions is checked but not recognised subcommand)

Not sure I understand this. If your concern is that a reader might think there is a subcommand check below, then we could rephrase the comment to

      // No subcommand will be dispatched if this line is reached.
      // If using passThroughOptions, stop processing as soon as recognised options end.

Technically, those are two completely separate comments. Could even put an extra line break between them.

I'll have another go at my abandoned attempt. How about:

// If using passThroughOptions, pass through the remaining args (including options) since have found something which is not a recognised option

A combination like

      // If using passThroughOptions and no subcommand has been found,
      // stop processing as soon as an argument that is not a recognised option is encountered.
      // All remaining arguments (including options) are treated as command-arguments and passed through
      // unless the first one is an option, in which case it is reported as unknown.

with the last line being optional would be nice. Might be a little too long, but I don't think it's a big problem as long as the explanation is clear and doesn't contain unnecessary details.

[aweebit]

Summary of my suggestion

…with a couple of new improvements to the comment.

      // If using passThroughOptions and no subcommand has been found (always the case when this line is reached),
      // stop processing as soon as an argument that is not a recognised option is encountered.
      // All remaining arguments (including options) are treated as command-arguments and passed through
      // unless the first one is an option, in which case it is reported as unknown
      // because dest is unknown in that case.
      if (this._passThroughOptions) {
        /* ... */

---

If I had managed to convince you in #1945, the last two line could be made more precise:

      // If using passThroughOptions and no subcommand has been found (always the case when this line is reached),
      // stop processing as soon as an argument that is not a recognised option is encountered.
      // All remaining arguments (including options) are treated as command-arguments and passed through
      // unless allowUnknownOption is off and the first argument is an option, in which case it is reported as unknown
      // because dest is unknown in that case.
      if (this._passThroughOptions) {
        /* ... */

---

If the functional overlap with enablePositionalOptions is removed as suggested in #1947:

      // If using passThroughOptions and no subcommand has been found,
      // stop processing as soon as an argument that is not a recognised option is encountered.
      // All remaining arguments (including options) are treated as command-arguments and passed through
      // unless the first one is an option, in which case it is reported as unknown
      // because dest is unknown in that case.
      if (this._passThroughOptions && !reprocessedBySubcommand) {
        /* ... */

Here, reprocessedBySubcommand is like in #1934 and #1946.

---

A variant for #1934:

      // If using passThroughOptions and no subcommand has been found (always the case when this line is reached),
      // stop consuming options except help as soon as an argument that is not a recognised option is encountered.
      // Only consume the help option if found before command-arguments.
      // Stop processing and exit the loop as early as possible.
      // All remaining arguments (including options) are treated as command-arguments and passed through
      // unless the first one is an option, in which case it is reported as unknown
      // because dest is unknown in that case.
      if (this._passThroughOptions) {
        onlyConsumeHelpOption = true;
        if (!this._hasHelpOption || !isArgFlag) {
          dest.push(arg, ...args);
          break;
        }
      }

---

With all the tweaks combined (my dream):

      // If using passThroughOptions and no subcommand has been found,
      // stop consuming options except help as soon as an argument that is not a recognised option is encountered.
      // Only consume the help option if found before command-arguments.
      // Stop processing and exit the loop as early as possible.
      // All remaining arguments (including options) are treated as command-arguments and passed through
      // unless allowUnknownOption is off and the first argument is an option, in which case it is reported as unknown
      // because dest is unknown in that case.
      if (this._passThroughOptions && !reprocessedBySubcommand) {
        onlyConsumeHelpOption = true;
        if (!this._hasHelpOption || !isArgFlag) {
          dest.push(arg, ...args);
          break;
        }
      }

[shadowspawn]

The related PR has already been rejected. To a large extent, I don't like .allowUnknownOptions() as a weak historical feature, and not concerned about documenting its effects!

Thank you for your contributions.