Skip to content
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

Do not set boolean to false if flag is not in args #116

Closed
pvdlg opened this issue Feb 16, 2018 · 14 comments
Closed

Do not set boolean to false if flag is not in args #116

pvdlg opened this issue Feb 16, 2018 · 14 comments

Comments

@pvdlg
Copy link
Contributor

pvdlg commented Feb 16, 2018

Currently when defining a boolean option, without specifying its default value, if the user doesn't set the corresponding arg the value is set to false.

Therefore its impossible to know if users set the flag to false or this they just omitted it.

This is problematic in situation in which you want to merge the options returned by yargs-parser with another object options (from a config file for example) with Object.assign or similar.
Typically you would merge options so the CLI args take precedence over the config file. So users can specify their config and override it in development mode by passing an option to the cli.

In such situation, if the user do not pass the flag in the CLI, the merged options will always have false for this flag, as yargs-parser option have it set to false and it override what's in the config.

Ideally the value of a boolean flag should be undefined when the user doesn't pass the flag in the CLI.

Would be open to a PR that implement such behavior?

@plroebuck
Copy link

You already have this capability if you don't like the default processing of booleans. Add
.default(key, undefined) to options you want to distinguish between user-provided vs. default.
Then filter out the undefined values prior to merging with your config.

@pvdlg
Copy link
Contributor Author

pvdlg commented Mar 9, 2018

Yes. However it feels like a workaround.
When you don't set a default value for boolean flag it feels weird that it's set to false. This proposal is about changing the default behavior:

  • By default unset boolean value are not part of the return object
  • If users want false for unset boolean the can use .default(key, false)

That would avoid to use the workaround of filtering out properties that are undefined.

@plroebuck
Copy link

Bool·e·an
noun COMPUTING

  1. a binary variable, having two possible values called “true” and “false.”

IMO, don't believe default behavior should be to create a third value, undefined, meaning the user didn't say. I understand a reason for being able to do so, but changing the default would be counterintuitive given the very definition of the word.

@pvdlg
Copy link
Contributor Author

pvdlg commented Mar 10, 2018

The objective of yargs-parser is to parse the user input in a meaningful way, not to adhere to a particular dictionary definitions.

In the context of a CLI, a flag (as in a parameter without value like --flag) can be enabled, disabled or omitted. So there is a difference in the user intent between those 3 inputs:

  • command --flag => User wants the force flag
  • command --no-flag => User wants to force not using the flag
  • command => User want to use the value from the config file

So the fact that yargs-parser calls it boolean might be semantically incorrect according to the strict definition of a boolean, and it could be named flag or something else instead, but that's beside the point.

@plroebuck
Copy link

Look, I totally understand what you're trying to do -- just disagree with changing what is known as default processing for everyone. However, it seems to me as though you'd have the same problem for all types, not just boolean, wouldn't you? Maybe use the config file to create the default values for the CLI processing (use output of first yargs invocation as options() for second yargs call). BTW, you didn't say -- your config file like a .rc file, the "config" section of package.json, or something else?

It's definitely using yargs at the advanced level. If there's no example code demonstrating how this could be achieved, it would definitely be of value. Might as well go the whole distance: hardcoded app-defaults, config file, environment variables, and finally CLI.

@pvdlg
Copy link
Contributor Author

pvdlg commented Mar 10, 2018

Look, I totally understand what you're trying to do -- just disagree with changing what is known as default processing for everyone.

I don't know, it seems you are trying to find every possible reason to push back on this proposal. If you disagree and in your opinion it's a bad idea, that's alright.
And yes that would be breaking change as it would change the current default behavior. That's why I opened this issue to make the proposal and get feedback from the maintainer team before implementing a PR.

However, it seems to me as though you'd have the same problem for all types, not just boolean, wouldn't you?

No, for all other types yargs-parser do not include properties in the resulting object for the options the user did not set. It's only the "boolean" type that behave differently than anything else. The proposal is to harmonize yargs-parser across all option types.

With this proposal yargs-parser would behave consistently for all types:

  • if the user passes the option in the CLI => set the value in the resulting object
  • if the user do not pass the option in the CLI and there is no default set => do not set the value in the resulting object
  • if the user do not pass the option in the CLI and there is a default set => set the default value in the resulting object

Maybe use the config file to create the default values for the CLI processing (use output of first yargs invocation as options() for second yargs call)

It seems to be a really complex workaround. At least way more complex than filtering out the undefined values.

BTW, you didn't say -- your config file like a .rc file, the "config" section of package.json, or something else?

It doesn't matter here. The use case mentioned is to merge the yargs-parser with another object that come from somewhere else unrelated to yargs-parser. The point of the proposal is to have the ability to know if the user has passed a flag or not, the same way it can be done for any other option type.

It's definitely using yargs at the advanced level. If there's no example code demonstrating how this could be achieved, it would definitely be of value. Might as well go the whole distance: hardcoded app-defaults, config file, environment variables, and finally CLI.

I don't really get what's the point of this remark. This proposal is exactly the opposite of hardcoding, as it propose to not hardcode the default value of boolean anymore (it's currently hardcoded in yargs-parser as false).

@plroebuck
Copy link

I don't know, it seems you are trying to find every possible reason to push back on this proposal. If you disagree and in your opinion it's a bad idea, that's alright.

Well, I have no research to back me up, but your implemented proposal would likely break all packages depending on yargs for CLI processing of boolean flags. That by itself should give pause to the idea. As such, I disagree about making it the default -- think breaking other people's dependent packages should always be a last resort.

That said, I have no problem at all with having a yargs-parser configuration option (a la camel-case-expansion) that provides the functionality you describe. Package authors needing this functionality could set a single flag in the configuration section of their package.json.

I don't really get what's the point of this remark. This proposal is exactly the opposite of hardcoding, as it propose to not hardcode the default value of boolean anymore (it's currently hardcoded in yargs-parser as false).

Perhaps I wasn't clear. When I said "hardcoded app-defaults", I meant those of the application itself, passed via yargs.options and provided by package author.

@evocateur
Copy link
Contributor

Well, I have no research to back me up, but your implemented proposal would likely break all packages depending on yargs for CLI processing of boolean flags.

How does this break the trivial falsey check (if (argv.foo) console.log("you passed --foo");)? I fail to see how this would be a breaking change in practice for the most common yargs idioms.

Even if folks are doing non-coercive comparison of these properties, bump the major and done. Nobody gets broken, everyone is happy. (The strict comparison would only be useful to distinguish a literal false if this proposal was implemented)

@plroebuck
Copy link

@evocateur Never said it would break trivial false checks, but I don't know if everyone's code processes cli options coercively. I advocated implementing the feature using a yargs-parser configuration option that wouldn't break anyone's code regardless. Your problem with that is what exactly?

@evocateur
Copy link
Contributor

evocateur commented Mar 24, 2018 via email

@plroebuck
Copy link

@pvdlg, looks like .boolean() defaulted false is causing problems for .conflicts() on the yargs side of the house as well now...

@ljharb
Copy link
Contributor

ljharb commented Mar 30, 2018

Regarding the boolean definition comment above; every enum for non-required user-supplied values always needs an additional option for “not provided” - this doesn’t make it not a boolean. The arg is a boolean, when provided, but the property on argv need not be.

@bcoe
Copy link
Member

bcoe commented Mar 31, 2018

@ljharb @evocateur @plroebuck sorry, managed to miss a fairly heated discussion about default boolean values it turns out.

I don't have a super strong opinion, as @evocateur points out undefined would still be falsy. @pvdlg if you feel strongly about this, perhaps make a patch that switches the behavior, and we'll see how much the test suites of yargs and yargs-parser would blow up? as @evocateur points out we can always release this as a major.

@pvdlg
Copy link
Contributor Author

pvdlg commented Mar 31, 2018

@bcoe see PR #119 with the change.
Only 2 tests are impacted. They explicitly verify the previous behavior (setting false when not defined in the CLI args).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants