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 define unset boolean args #77

Merged
merged 5 commits into from
Apr 25, 2018

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Feb 15, 2018

Proposal to solve xojs/xo#299.

For each boolean arg that doesn't define a default, set it to undefined. After parsing remove the options with a value of undefined.

Unfortunately this doesn't work with minimist due to an unrelated bug when using both default and alias:

minimist(['-f'], {boolean: ['foo'], alias: {f: 'foo'}, default: {foo: null}});
// => {_: [], foo: true, f: [null, true]}
// Should be {_: [], foo: true, f:  true}

minimist(['--foo'], {boolean: ['foo'], alias: {f: 'foo'}, default: {foo: null}});
// => {_: [], foo: true, f: [null, true]}
// Should be {_: [], foo: true, f:  true}

I replaced minimist with yargs-parser to solve this issue.

meow relies on minimist-options to transform the options format. It still works with yargs-parser as the format is the same.

This PR works as a fix for xojs/xo#299 but it might be a desireable to support the other yargs-parser options. That would require to support those option in minimist-options or to replace it.

Not sure if it worth the effort though, as yargs already allow all those options and support the same convenient format as meow via the options function:

const argv = require('yargs')
    .options({
      rainbow: {
	type: 'boolean',
	alias: 'r'
     }
   }).argv;

@sindresorhus
Copy link
Owner

Does Yargs also default to undefined if boolean is the type and no default is specified?

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 15, 2018

Yards default to false like minimist.
For some reasons minimist ignore {default: undefined} but works with {default: null}.

@sindresorhus
Copy link
Owner

I'm still a little bit unsure whether undefined when no default is specified for boolean is the best default. It's quite convenient not having to specify default: false for all boolean flags. Maybe we should make this behavior opt-in with a Meow option?

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 15, 2018

OK, we can have an option. We just need to define what's the default.
What is most the common case:

  • Wanting the unset boolean arg to not be included in the result options object?
  • Or wanting the unset boolean args to be false?

@sindresorhus
Copy link
Owner

Or wanting the unset boolean args to be false?

This. I've only needed unset options in XO and one other CLI before.

@sindresorhus
Copy link
Owner

Even when boolean defaults to false, it should still be possible to unset them individually, by setting default: null or default: undefined.

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 15, 2018

Even when boolean defaults to false, it should still be possible to unset them individually, by setting default: null or default: undefined.

That's already the case in the current PR.

So we can add the option booleanDefault, and whatever value set there will be used as default, unless a default is already used.
By default booleanDefault will be false.

@sindresorhus
Copy link
Owner

So we can add the option booleanDefault, and whatever value set there will be used as default, unless a default is already used.
By default booleanDefault will be false.

Yes! Much better than what I had in mind.

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 18, 2018

I added the booleanDefault option. By default it's false so the default behavior doesn't change.

@pvdlg pvdlg changed the title Do not defined unset boolean args Do not define unset boolean args Apr 4, 2018
@pvdlg
Copy link
Contributor Author

pvdlg commented Apr 4, 2018

Updated to to yargs-parser@10.0.0

@sindresorhus sindresorhus merged commit 09f7ef0 into sindresorhus:master Apr 25, 2018
@sindresorhus
Copy link
Owner

sindresorhus commented Apr 25, 2018

Looks good. Thanks, @pvdlg :)

@pvdlg pvdlg deleted the unset-boolean-args branch April 25, 2018 03:14
tommy-mitchell added a commit to tommy-mitchell/meow that referenced this pull request Jan 28, 2024
- this was introduced in sindresorhus#77, likely due to compatibility with minimist
- `booleanDefault: null` causes minimist-options to error
- even in 2019, this was a confusing type:
  sindresorhus#116 (comment)
tommy-mitchell added a commit to tommy-mitchell/meow that referenced this pull request Feb 29, 2024
* this was introduced in sindresorhus#77, likely due to compatibility with minimist
* `booleanDefault: null` causes minimist-options to error
* even in 2019, this was a confusing type:
sindresorhus#116 (comment)
tommy-mitchell added a commit to tommy-mitchell/meow that referenced this pull request Feb 29, 2024
* this was introduced in sindresorhus#77, likely due to compatibility with minimist
* `booleanDefault: null` causes minimist-options to error
* even in 2019, this was a confusing type:
sindresorhus#116 (comment)
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

Successfully merging this pull request may close these issues.

2 participants