complain about unknown options if there are >0 arguments on command line

[clarsen]

As per #921 this passes unknown to command:* listener and as a result, the default command will also complain about unknown options. Before, it wouldn't.

The test in test.arguments.js needed to be adjusted since it would silently succeed even though an unknown option --setup_mode was passed and eaten.

[shadowspawn]

This looks like the same code change as #654, although for different reasons.

Note to self: see also useful comment from @clarsen in issue: #921 (comment)

[shadowspawn]

#654 has been closed, so this PR is unblocked

[shadowspawn]

Open related issue is #561. [Edit: now think #561 is different, see next comment.]

[shadowspawn]

After deeper analysis I discovered that there is different unknown option detection depending on whether there is an action handler attached to the program. This PR detects the unknown option when there is an action handler, but does not change the behaviour for the original report in #561 which needs a different fix.

[shadowspawn]

I think I understand! The option handling is very subtle. This fix covers the case when there is an action handler attached to the program (or default command) object and makes the behaviour consistent with subcommands with action handlers. I think this is reasonable.

It is a potentially breaking change in behaviour so needs to wait for a major release. There is an argument that ignoring options was a bug, but it has been that way for a long time and some people may have assumed it was intended and be relying on it.

I want to change the fix to the test, and I'll add a review about that.

@clarsen are you still around and interested in doing more work on this, or shall I take it over?

[shadowspawn]

Reminder to myself:

• add example code and fixed behaviour while I understand
• add review about test, remove --setup_mode

[clarsen]

@clarsen are you still around and interested in doing more work on this, or shall I take it over?

I've been quietly watching the discussion on this but don't have a deep understanding of the legacy behavior issues. Certainly would like to contribute back to this...

[shadowspawn]

This PR makes the error handling for an action attached to the program work the same way as an action attached to a subcommand. In particular unknown options now cause an error even when an argument is supplied.

Given a program:

const program = require("commander");

program
  .arguments("<file>")
  .action(function (fileParam) {
    console.log(`program called with: ${fileParam}`);
  });

program
  .parse(process.argv);

Before:

$ node pm0.js 
program
$ node pm0.js --silly
error: unknown option '--silly'
$ node pm0.js x
program
# And the one that changes...
$ node pm0.js x --silly
program

After:

$ node pm0.js 
program
$ node pm0.js --silly
error: unknown option '--silly'
$ node pm0.js x
program
# Now checks the options
$ node pm0.js x --silly
error: unknown option '--silly'

[shadowspawn]

The old test was making assumptions that unknown options always take a value (which is assuming too much, since it could be a plain boolean flag.)

Rather than adding a command to make the old call work, I would prefer to remove the option which is now being detected and rejected. So just a one line change to the original.

So was:

program.parse(['node', 'test', '--config', 'conf1', 'setup', '--setup_mode', 'mode3', 'env1']);

Suggestion dropping the now invalid option:

program.parse(['node', 'test', '--config', 'conf1', 'setup', 'env1']);

i.e. this particular test is meant to be testing .arguments

[clarsen]

@shadowspawn Thanks... I'll try using this in the CLI app I had that originally motivated the change to check it works 'ok'.

[shadowspawn]

I am going to pick this up and rework after the move to Jest. I'll add a co-author on the commit.

[shadowspawn]

Closed with implementation moved to #1049

[shadowspawn]

#1049 has been merged to develop and will be released in v4.0.0.

Thank you for your contributions.

[clarsen]

#1049 has been merged to develop and will be released in v4.0.0.

Thanks, I verified that commander@4.0.0-1 (prerelease) properly warns about unknown options with one of my scripts. I'll wait until the final release before switching over and deleting my fork...

[shadowspawn]

v4.0.0 has been released.

Thank you for your contributions.

[shadowspawn]

(Accidentally opened branch making comment.)