Prevent options with flags already in use from being added by aweebit · Pull Request #1923 · tj/commander.js

aweebit · 2023-07-31T23:13:43Z

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 · 2023-08-01T00:11:06Z

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 · 2023-08-01T00:12:33Z

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

aweebit · 2023-08-01T06:39:04Z

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 · 2023-08-01T07:22:35Z

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 · 2023-08-01T07:51:02Z

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 · 2023-08-01T08:01:29Z

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.

Co-authored-by: John Gee <john@ruru.gen.nz>

aweebit · 2023-08-01T08:28:05Z

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 · 2023-08-01T08:42:24Z

lib/command.js

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


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.

_registerOption() has been borrowed from deb3bdf in tj#1923. Watch out for merge conflicts!

shadowspawn · 2023-08-07T03:55:50Z

(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.)

- Check if new option is usable with current settings before registering - Check if registered options remain usable when updating settings _registerOption() has been borrowed from deb3bdf in tj#1923.

shadowspawn · 2023-10-30T09:10:57Z

Carried on in #2055

Prevent adding options with flags already in use

696f1e2

This comment was marked as resolved.

Sign in to view

shadowspawn added the semver: major Releasing requires a major version bump, not backwards compatible label Aug 1, 2023

Use _findOption() to check for flags already in use

5ec2249

This comment was marked as resolved.

Sign in to view

aweebit added 2 commits August 1, 2023 09:53

Simplify check for flags already in use

c1defc8

Use format imposed by VSCode formatter

c3f3450

aweebit mentioned this pull request Aug 1, 2023

Prevent aliases already in use and subcommands with such an alias or the name already in use from being added #1924

Closed

Simplify check for flags already in use

9721ae8

Co-authored-by: John Gee <john@ruru.gen.nz>

aweebit mentioned this pull request Aug 1, 2023

Clean up state from previous parse call when calling parse() / parseAsync() #1919

Closed

shadowspawn reviewed Aug 1, 2023

View reviewed changes

shadowspawn changed the base branch from develop to release/12.x August 1, 2023 08:46

Fix linting error

5c87223

shadowspawn requested changes Aug 1, 2023

View reviewed changes

This was referenced Aug 2, 2023

Enforce correct option usage and support custom option flag and name extraction in helpOption() #1926

Closed

Warn about obscured help option #1931

Closed

Prevent adding version option with flags already in use

deb3bdf

aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 3, 2023

Warn about help option obscured by version option

eacaee8

_registerOption() has been borrowed from deb3bdf in tj#1923. Watch out for merge conflicts!

This was referenced Aug 4, 2023

Allow dynamically updating version number and version option flags and description #1933

Closed

Add missing check for broken passThrough #1937

Merged

shadowspawn added the on hold label Aug 8, 2023

This was referenced Aug 13, 2023

Pull request catalog by @aweebit #1964

Closed

Make command-argument array public (in readonly mode) #1970

Closed

shadowspawn removed the on hold label Sep 4, 2023

shadowspawn mentioned this pull request Oct 30, 2023

Throw error when add option with clashing flags #2055

Merged

shadowspawn closed this Oct 30, 2023

Uh oh!

Conversation

aweebit commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request

Problem

Demo

ChangeLog

Changed

Peer PRs

…solving similar problems

Uh oh!

shadowspawn commented Aug 1, 2023

Uh oh!

shadowspawn commented Aug 1, 2023

Uh oh!

This comment was marked as resolved.

aweebit commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

shadowspawn commented Aug 1, 2023

Uh oh!

aweebit commented Aug 1, 2023

Uh oh!

shadowspawn commented Aug 1, 2023

Uh oh!

aweebit commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shadowspawn Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

shadowspawn left a comment

Choose a reason for hiding this comment

Uh oh!

shadowspawn commented Aug 7, 2023

Uh oh!

shadowspawn commented Oct 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aweebit commented Jul 31, 2023 •

edited

Loading

aweebit commented Aug 1, 2023 •

edited

Loading

aweebit commented Aug 1, 2023 •

edited

Loading