Prevent options with flags already in use from being added

[aweebit]

Pull Request

Problem

Adding options with flags that are already in use is allowed.

Demo

const { Command } = require('commander');

var program = new Command();
program
  .option('-a, --first')
  .option('-a, --second');
program.parse(['-a'], { from: 'user' });
console.log(program.opts()); // { first: true }

var program = new Command();
program
  .option('-V, --version [arg]', 'added via .option()')
  .version('1.0.0')
  .action(() => {
    console.log('called action handler');
    console.log('version value is', program.opts().version);
  });
program.outputHelp(); // both version option and option added via .option() are shown
program.parse(['-V', '2.0.0'], { from: 'user' }); // action handler not called

ChangeLog

Changed

• Breaking: throw an error when .addOption() is called with an option whose flags are already in use

Peer PRs

…solving similar problems

• Prevent aliases already in use and subcommands with such an alias or the name already in use from being added #1924
• Warn about obscured help option #1931 (introduces _registerOption(), too – watch out for merge conflicts!)

[shadowspawn]

Commander only checks some of the possible mistakes/accidental-misuse by the "author", but willing to consider more as they come up.

Related: #1903

[shadowspawn]

I am hopeful this can be done with less code by using the internal Command._findOption.

[aweebit]

I am hopeful this can be done with less code by using the internal Command._findOption.

Done in 5ec2249. Further simplifications would make the error message less detailed.

[shadowspawn]

The way an option is usually referred to in the end-user messages is '${option.flags}' and we can use same format in author facing error. My reasoning for this format is the combo is potentially more recognisable than a single flag.

I suggest listing the found item in same format too. I am deliberately simplifying here and not doing the error breakdown you did (although I admire the effort). I think it will be clear enough to the author?

    const matchingOption = (option.short && this._findOption(option.short)) || (option.long && this._findOption(option.long));
    if (matchingOption) {
      throw new Error(`Cannot add option '${option.flags}' as an option using same flag has already been been added
- conflicts with option '${matchingOption.flags}'`);
    }

[aweebit]

The way an option is usually referred to in the end-user messages is '${option.flags}' and we can use same format in author facing error. My reasoning for this format is the combo is potentially more recognisable than a single flag.

A combo is already used, but with option-argument and extra spaces removed: see flagString on line 510. I think it is better than what you suggest because the actual flags are the only thing that matters here.

[shadowspawn]

I did forget you were using one combo! But no need to calculate it, just use '${option.flags}' which matches what the author specified and other messages.

I think identifying the conflicting option is important and the whole matchingOption.flags is easier to find than a flag alone.

[aweebit]

Fair enough. I used your suggestion in 9721ae8, just added the command name and changed "as" to "since" because it reads better (with "as", I first read it as (Cannot add option '${option.flags}' as an option using same flag) has already been added and was confused).

[shadowspawn]

command name is good addition 👍

[shadowspawn]

Code looks good to me, simple and informative and effective.

Just needs a couple of tests, for short and long conflicts.

[shadowspawn]

(I am concerned this may be a blocking change for people upgrading big programs from older versions of Commander, because their code will be broken until they resolve the clashes. I might want to add configuration to make this optional specifically to ease migration, but won't do that now. We can do a prerelease and see if people hit issues that they considered an obstacle.)

[shadowspawn]

Carried on in #2055