-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(option): allow to set options as conflicting #1678
Conversation
Only one previous request I am aware of: #1408 |
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 Support in some other cli parsers: |
I was imagining these sorts of routines would identify the target option(s) using the cli flags, like You have used the option name as the key. Interesting! There is client facing code that works this way, like |
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:
With |
No recommendations yet, just exploring the problem. 🤔 🔍 |
Should the exclusions be listed in the help? (i.e. |
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. |
One last complication to be aware of: negated options, like |
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! |
I was wondering what a hook check might look like. Using explicit test rather than generalised (and using 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();
|
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. |
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). 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.
Can you elaborate on this? I'm understanding that testing for value, implies testing for existence too |
Good point, let me experiment with this a bit |
True! Do we have a way to know if that was the case? I suppose we can add a |
We can either have |
Very cool, I assume I can create a utility hook and re-use it in various commands based on the logic in this PR. |
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. |
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 |
Conveniently, support for this was added recently. See |
In the case where there is a lone negated option, like In the case where there is a dual option with a positive ( |
@shadowspawn, I rebased the PR and followed up with better error reporting and help information. |
Thanks, I will take a deeper look and give it a try. |
I like the clarity of this error involving an option coming from an environment variable:
The author may want command-line arguments to "win" against environment variables, but in that case they can do it themselves and not use |
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
(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 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
|
A possible implementation, which as a bonus makes repeated calls accumulate:
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.) |
@shadowspawn great point as I didn't notice that error as well. Let me know if 0059715 is what you had in mind. |
Yes, nice. 👍 |
We have given other people a couple of weeks for commenting after mentioning this on the Over to you @abetomo for a look, when you have time. 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
LGTM.
Thanks for all the hard work @erezrokah. This can go out in a minor release, probably within the next few weeks. |
Thanks @abetomo and @shadowspawn for the review. |
@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
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:
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,
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. |
@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! |
If you think my point has merit, I can open a fresh issue if you would like. |
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 |
Makes sense. Another option might be to make explicit the two different roles that an environment variable might play, e.g.
or maybe just an boolean:
The nice things about this solution are:
A
The other way to go is to do this in
Or it could work off the precedence hierarchy of option sources (CLI > ENV > default). If
Just some ideas :) |
Pull Request
Hello 👋 Thank you for this lovely package, this PR implements exclusive options (see details below).
Happy to rename it to(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).conflicts
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.ChangeLog
feat(option): allow to configure conflicting options