feat(option): allow to set options as conflicting

[erezrokah]

Pull Request

Hello 👋 Thank you for this lovely package, this PR implements exclusive options (see details below).
Happy to rename it to conflicts (edit: done) or anything else you find suiting, and of course open to any kind of feedback (especially if you think this should be user code and not package code).
I searched previous issues and PRs and could only find this related comment: #1358 (comment)

Problem

I would like to configure options that are mutually exclusive. For example --json (to set the output type), conflicts with --silent (to suppress output).

Related to #1358 (comment)

Solution

Added a new option method conflicts that accepts an array of strings (or a single string) with options that conflict with the configured option.

• TypeScript typings
• JSDoc documentation in code
• tests
• README
• examples/

ChangeLog

feat(option): allow to configure conflicting options

[shadowspawn]

Only one previous request I am aware of: #1408

[shadowspawn]

Thinking about pattern of usage. This could be applied at the command level with a set which are mutually exclusive, or could be directional and per option with x excludes y. Looking at other packages I can see both, but x->y is more common that I expected. (Which is convenient, because it is less intrusive adding less common functionality down in Option!)

Support in some other cli parsers:

• argparse has add_mutually_exclusive_group which is at the command level and applied to subsequently added options

• oclif has exclusive which is x->y

• yargs has conflicts which is x->y

[shadowspawn]

I was imagining these sorts of routines would identify the target option(s) using the cli flags, like .exclusive('--json'). There isn't other client facing code that works this way so I didn't particularly like that idea.

You have used the option name as the key. Interesting! There is client facing code that works this way, like .getOptionValue(key).

[shadowspawn]

An interesting question is whether a conflict is if both options are specified, or both options have values. In other words, can an excluded option have a default value. Related issues in other parsers:

• conflicts problem yargs/yargs#1910
• Conflicting options with default values (or boolean values) yargs/yargs#929
• default impacting dependsOn/exclusive oclif/oclif#211

With .requiredOption() the test is whether the option has a value. (And predates being able to tell where a option value came from. Hmm, maybe requiredOption should ignore the default value these days too!)

[shadowspawn]

No recommendations yet, just exploring the problem. 🤔 🔍

[shadowspawn]

Should the exclusions be listed in the help? (i.e. .optionDescription())

[shadowspawn]

The value triggering the exclusive check can potentially come from an environment variable rather than the CLI. This might be difficult to reasonably capture in the error message.

[shadowspawn]

One last complication to be aware of: negated options, like --no-colour

[shadowspawn]

Thanks for a tidy description of PR. 👍

Prompted lots of thoughts, I'll stop the stream of comments for now and think about it for a bit!

[shadowspawn]

I was wondering what a hook check might look like. Using explicit test rather than generalised (and using .error() from Commander 9):

const { Command, Option } = require('commander');
const program = new Command();

program
  .addOption(new Option('-p, --port <number>', 'port number').env('PORT'))
  .addOption(new Option('-d, --disable-server', 'disables the server')) // .exclusive(['port']));
  .hook("preAction", (thisCommand, actionCommand) => {
    const options = actionCommand.opts();
    if (options.port !== undefined && options.disableServer !== undefined)
      actionCommand.error("error: option '-program, --port <number>' cannot be used with '-d, --disable-server");
  })
  .action (() => {});

program.parse();

% node index.js --disable-server --port 8000
error: option '-program, --port <number>' cannot be used with '-d, --disable-server
% PORT=23 node index.js --disable-server    
error: option '-program, --port <number>' cannot be used with '-d, --disable-server

[erezrokah]

Thinking about pattern of usage. This could be applied at the command level with a set which are mutually exclusive, or could be directional and per option with x excludes y. Looking at other packages I can see both, but x->y is more common that I expected. (Which is convenient, because it is less intrusive adding less common functionality down in Option!)

Very good thought. My take is that you have more context on exclusive options when you add a new option, and not when creating a branch new command.
What was counterintuitive for me at first is that the usage is at the option level, and the implementation is at the command level (since we need to look at all the options to determine violations).

[erezrokah]

An interesting question is whether a conflict is if both options are specified, or both options have values. In other words, can an excluded option have a default value. Related issues in other parsers:

Great point. I didn't consider default values and required options.

I would say a conflict exists if both are specified, regardless where the value comes from (e.g. user, default or required).
Required options or options with a default value should not be able to be configured as exclusive, and we should print a relevant error message for it (I can add that logic to the PR if you agree).

The reasoning is that the purpose of this code is to save users from adding custom validation code. If we were to say that setting a default value is not conflicting, users will still need to do some custom validation to handle that specific case.
I believe this matches yargs/yargs#929 (comment)

With .requiredOption() the test is whether the option has a value.

Can you elaborate on this? I'm understanding that testing for value, implies testing for existence too

[erezrokah]

Should the exclusions be listed in the help? (i.e. .optionDescription())

Good point, let me experiment with this a bit

[erezrokah]

The value triggering the exclusive check can potentially come from an environment variable rather than the CLI. This might be difficult to reasonably capture in the error message.

True! Do we have a way to know if that was the case? I suppose we can add a valueOrigin property

[erezrokah]

One last complication to be aware of: negated options, like --no-colour

We can either have exclusive: ['color', 'no-colour'] or have colour imply no-colour.
Not sure what's better actually, I lean towards implying no-colour for the same reasoning as in #1678 (comment)

[erezrokah]

I was wondering what a hook check might look like. Using explicit test rather than generalised (and using .error() from Commander 9):

Very cool, I assume I can create a utility hook and re-use it in various commands based on the logic in this PR.

[shadowspawn]

I would say a conflict exists if both are specified, regardless where the value comes from (e.g. user, default or required).
Required options or options with a default value should not be able to be configured as exclusive, and we should print a relevant error message for it (I can add that logic to the PR if you agree).

We don't tend to warn for broken combinations if author will easily discover for themselves. And I think required+exclusive is fairly obvious. (But I am not against warning about broken combinations that are subtle.)

default+exclusive can work, as per other comments.

[shadowspawn]

With .requiredOption() the test is whether the option has a value.

Can you elaborate on this? I'm understanding that testing for value, implies testing for existence too

An option value may come from the default, or from an environment variable, or from the command-line (or from custom code). The validation for .requiredOption() just checks that the option has a value after parsing. From any source.

[shadowspawn]

True! Do we have a way to know if that was the case? I suppose we can add a valueOrigin property

Conveniently, support for this was added recently. See . getOptionValueSource().

[shadowspawn]

One last complication to be aware of: negated options, like --no-colour

We can either have exclusive: ['color', 'no-colour'] or have colour imply no-colour. Not sure what's better actually, I lean towards implying no-colour for the same reasoning as in #1678 (comment)

In the case where there is a lone negated option, like --no-colour, the option key is colour and I think it might Just Work like other options (assuming default values are ignored, since there is an implicit value of true). It reads a confusing way (.exclusive('colour')) but arguably still means the option value for colour may not be specified if this option is specified. Not sure I have convinced myself!

In the case where there is a dual option with a positive (--colour) and negated (--no-colour) I don't think we currently have enough information to definitively tell which supplied the value. We could muck around and take a good guess, but I am tempted not to add special code and leave that in the too-hard basket for now.

[erezrokah]

@shadowspawn, I rebased the PR and followed up with better error reporting and help information.
Please let me know if I missed anything

[shadowspawn]

Thanks, I will take a deeper look and give it a try.

[shadowspawn]

I like the clarity of this error involving an option coming from an environment variable:

% PORT=80 node index.js  -d
error: option '-d, --disable-server' cannot be used with environment variable 'PORT'

The author may want command-line arguments to "win" against environment variables, but in that case they can do it themselves and not use exclusive.

[shadowspawn]

I wanted to experiment with adding to the help, thanks for doing that. Having seen an example and thought about it some more, I think I would prefer not to add to the help!

The full-sentence way of referring to exclusive is a bit different, and exclusive on its own does not scan as well. Comparing:

The options --red and --blue are mutually exclusive.

   --red        (exclusive: blue)

(Could be improved a bit with tweaking the wording.)

The help could use the short option (to save space) or the long option (as more meaningful) or both, but I don't think it can use the attribute name as that is an internal detail. In other words, the author knows that --foo-bar is option fooBar but the command-line user may not.

In many cases it will be reasonably obvious to the user that the options don't make sense used together.

The net result is I think it is better to leave it to the author to include the information themselves, if they wish.

---

I checked yargs and argParse and they do not add info to the help for the option. Credit to argParse though, it does a full synopsis and does include the mutual exclusion as [--red | --blue]:

% node exclusive.js -h             
usage: PROG [-h] [--red | --blue]

optional arguments:
  -h, --help  show this help message and exit
  --red
  --blue

[shadowspawn]

I had wondered about whether to support a single string being passed to .conflicts() rather than an array. I wasn't going to raise it until I noticed I made that very mistake in one of my examples!

  .addOption(new Option('--dual2 <str>').conflicts('red'))

It does not fail at runtime, but is doing a string includes rather than an array includes in the conflict testing and not actually correct.

[shadowspawn]

A possible implementation, which as a bonus makes repeated calls accumulate:

  conflicts(names) {
    if (!Array.isArray(names) && typeof names !== 'string') {
      throw new Error('conflicts() argument must be a string or an array of strings');
    }

    this.conflictsWith = this.conflictsWith.concat(names);
    return this;
  }

This will change the parameter type in the JSDoc and Typescript, and need a few more tests too!

(If you think this is sensible but don't want to do it yourself, I am happy to try adding it.)

[erezrokah]

@shadowspawn great point as I didn't notice that error as well. Let me know if 0059715 is what you had in mind.

[shadowspawn]

Yes, nice. 👍

[shadowspawn]

We have given other people a couple of weeks for commenting after mentioning this on the .implies() issue and previous conflicts issue.

Over to you @abetomo for a look, when you have time. 😄

[abetomo]

Thanks for your contribution!
LGTM.

[shadowspawn]

Thanks for all the hard work @erezrokah. This can go out in a minor release, probably within the next few weeks.

[erezrokah]

Thanks @abetomo and @shadowspawn for the review.
I truly enjoyed the interaction and thoughtful feedback ❤️

[vassudanagunta]

@shadowspawn @erezrokah This is obviously late, but I came across this thread while evaluating Commander vs yargs. (Wouldn't it be great if there was a decision tree service, perhaps interactive, that would help people make such choices?)

Let's assume PORT=80 is set in the environment somehow (I don't want to set it on the same line as the command as in the test examples in previous comments because that is visually misleading). Since CLI options override environment variables (I assume this to be Commander's rule):

% node index.js -p 8080   # no conflict error, port is 8080, env is ignored

wouldn't it also be logical that a related "conflicting" option also have precedence over the same environment variable and not result in a conflict error? For example, default/ENV server-related values should simply irrelevant when the server is disabled, not generate a conflict error:

% node index.js --disable-server'  # no conflict, server disabled, related PORT env var is ignored

To me a default is not the same as an explicit value. It is included for convenience/usability/safety only. I would think that it is a very common occurrence, to continue to server example, for the program to have a default port that takes effect if and only if the command results in starting a server, without requiring the command to do so.

Yes,

The author may want command-line arguments to "win" against environment variables, but in that case they can do it themselves and not use exclusive.

but if the more common and intuitive behavior is as I describe, shouldn't that be the default behavior and authors can do themselves if they want otherwise?

Just a thought.

[vassudanagunta]

@shadowspawn, as I said I was only looking at this thread to help me decide which CLI library to adopt. What I was looking for is a sense of the maintainer(s), how they think, how they evaluate ideas, what kind of care and judgement they use. Just wanted to say I am highly impressed!

[vassudanagunta]

If you think my point has merit, I can open a fresh issue if you would like.

[shadowspawn]

The environment variables might get used for defaults, or they might be the primary way options are set. This second approach is common in container environments so you can modify the runtime without rebuilding the container.

There is some discussion around env in an earlier PR: #1266

We decided in this PR that conflicts would not consider default values. One big difference between simple defaults and env is that the author knows what the defaults are and why. The environment variables are coming from the end-user, who may be using them like defaults or like CLI options, and may or may not understand how they interact.

One work-around to get the behaviour you describe is to set the environment variables as the default value in code, and not use the .env() support. #1266 (comment)

[vassudanagunta]

Makes sense. Another option might be to make explicit the two different roles that an environment variable might play, e.g.

.addOption(new Option('-p, --port <number>').env('PORT', {asDefault: true}))

or maybe just an boolean:

.addOption(new Option('-p, --port <number>').env('PORT', true))

The nice things about this solution are:

• backward compatible: if the extra argument is not supplied, it defaults to treating the env var as an explicit setting, the same as today.
• simple explanation: "if asDefault is true, the environment value behaves exactly like a default value configured for an Option.
• simple implementation: if asDefault is true, uses the existing logic for Option defaults (which don't trigger conflicts).
• there may be other benefits, if there are other areas where the two different roles of environment variables should be treated differently by Commander.

A role: 'default' | 'explicit' | 'fixed' option would be more powerful and future proof:

//'fixed' means an env value can't be overridden even by a CLI arg. Here
// we don't want a CLI to override the fact that this is a production environment.
.addOption(new Option('--prod').env('PROD', {role: 'fixed'}).default(true))

---

The other way to go is to do this in conflicts. I don't think this is the way to go, it's not as elegant or intuitive, but just to throw it out there:

.addOption(new Option('--disable-server')
        .conflicts('port', {includeEnv: false}))

Or it could work off the precedence hierarchy of option sources (CLI > ENV > default). If usePrecedence is true, then a conflict only occurs if the two values are sourced at the same precedence level:

.addOption(new Option('--disable-server')
        .conflicts('port', {usePrecedence: true}))

Just some ideas :)