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

Fix "boolean" flag with "duplicate-arguments-array" #184

Merged
merged 3 commits into from Jun 24, 2019

Conversation

@juergba
Copy link
Contributor

@juergba juergba commented Jun 15, 2019

Description

var args = parse('--file --file', {
  boolean: ['file'],
  configuration: {
    'duplicate-arguments-array': true,
    'flatten-duplicate-arrays': false
  }
})          // { _: [], file: true }

The result is incorrect, it should be: { _: [], file: [ true, true ] }

The root problem can be found in an incorrect boolean handling of default values and hence a bug fix at the wrong place.

PR #119 shows the boolean option behavior when no default value is set.

var parse = require('yargs-parser');

parse('--flag', {boolean: ['flag']});
// => { _: [], flag: true }

parse('--no-flag', {boolean: ['flag']});
// => { _: [], flag: false }

parse('', {boolean: ['flag']});
// => { _: [] }                     => flag not set similarly to other option type

When a default value is set:

parse('--flag', {boolean: ['flag']});
// => { _: [], flag: true }                 => no default value should be set
                                            => interim result before setKey(): [false, true]

parse('--no-flag', {boolean: ['flag']});
// => { _: [], flag: false }

parse('', {boolean: ['flag']});
// => { _: [], flag: false }           => flag is set to default value

In the first case no default must be set since --flag as a boolean already implicitly carries its value true.

Description of Change

  • remove the setting of a default value when a boolean is parsed without value. A boolean option carries its implicit value true.
  • setKey(): allow boolean's to create array's
@juergba juergba changed the title WIP: Fix "boolean" option with "duplicate-arguments-array" Fix "boolean" flag with "duplicate-arguments-array" Jun 15, 2019
Copy link
Member

@bcoe bcoe left a comment

could use some minor points of clarification (mainly I'm worried about the fact that we remove the defaulted feature entirely, and that this might change the behavior of dot notation and arrays).

test/yargs-parser.js Outdated Show resolved Hide resolved
@@ -762,19 +752,7 @@ function parse (args, opts) {
return isSet
}

function setDefaulted (key) {
Copy link
Member

@bcoe bcoe Jun 16, 2019

I'm concerned that we're completely removing the setDefaulted functionality, which I see used here:

        if (((configOnly && flags.configs[keys.join('.')]) || !configOnly) && (!hasKey(argv, keys) || flags.defaulted[keys.join('.')])) {
          setArg(keys.join('.'), process.env[envVar])
        }

and, here:

        if (!hasKey(argv, fullKey.split('.')) || (flags.defaulted[fullKey]) || (flags.arrays[fullKey] && configuration['combine-arrays'])) {
          setArg(fullKey, value)
        }

If it's true that we're not using this functionality, awesome let's get rid of it -- I'm just slightly worried we might be missing a unit tests for this historic behavior ... I tried to perform a git blame and it looks like this feature has been around forever.

Copy link
Contributor Author

@juergba juergba Jun 17, 2019

var args = parse('', {
  boolean: ['file'],
  // default: {'file': true},
  configObjects: [{'file': false}]
})

before PR #119 the behavior was:

  • there is no --file in CLI and no user set default, but we set an imlicit false => file: false
  • set defaulted[file] = true
  • while processing configObect the user input overrides the defaulted value

after PR #119 the behavior is:

  • there is no --file in CLI and no user set default, so we don't set this property
  • therefore we don't set defaulted neither
  • while processing configObect the user input is set
  • in case of an user set default: applyDefaultsAndAliases() does this job for number, string and boolean the identical way.

IMO this defaulted can be explained with historic reasons and can be removed completely.
Now defaulted is not set anywhere.

Copy link
Contributor Author

@juergba juergba Jun 17, 2019

set-placeholder-key:
when truthy, setPlaceholderKeys() sets the value to undefinded, very similar to applyDefaultsAndAliases() which sets a user default value instead. So in this case we don't need defaultedneither.

Copy link
Member

@bcoe bcoe Jun 22, 2019

@juergba do you want to go ahead and submit a patch that completely removes the defaulted behavior then?

Copy link
Contributor Author

@juergba juergba Jun 23, 2019

yes, done.

@bcoe bcoe requested review from laggingreflex and aorinevo Jun 16, 2019
@juergba juergba force-pushed the issue/boolean-default branch from 923a3b1 to 3aa03f0 Jun 17, 2019
@aorinevo
Copy link

@aorinevo aorinevo commented Jun 17, 2019

As @juergba pointed out, if no default is set then Boolean flags work as desired. That is, when flag is present it is set to true; otherwise it is falsey.

Note that defaulted works as expected. The expectation when setting a default is that it is set for us. In this case, when Boolean is defaulted to false, I’d expect it to set the flag to false, equivalent to ‘—flag=false’, unless explicitly overridden, as in ‘—flag=true’.

That being said, I’ve come across this in the past and found it a bit strange. Perhaps better documentation for defaults?

Note, my comment is strictly focused on the behavior of Boolean defaults.

@aorinevo
Copy link

@aorinevo aorinevo commented Jun 17, 2019

@juergba, what happens when you pass ‘—file=true —file=true’?

@juergba
Copy link
Contributor Author

@juergba juergba commented Jun 17, 2019

@aorinevo I haven't understood wether you agree or disagree with my changes.

what happens when you pass ‘—file=true —file=true’?

result: { _: [], file: [ true, true ] }

In this case user defaults are not involved at all. --file=true is just split and flag is set to true. No defaults are required in this case.

@aorinevo
Copy link

@aorinevo aorinevo commented Jun 18, 2019

I’m open to this change as it is more intuitive and backwards compatible in the following sense: If default value is set to false, it can be overridden by either having the flag present and set to true (existing behavior) or just having the flag present (new behavior and could not have been used in this way prior to this PR.

This PR should also include updates to docs.

@juergba
Copy link
Contributor Author

@juergba juergba commented Jun 18, 2019

Actually I haven't changed the default behavior of boolean in any way. This was done by PR #119.

I'm trying to fix a bug which prevents the correct use of duplicate-arguments-array = true.
Current master:

  • --file: an implicit default (false) is set plus an implicit value (true) ==> [false, true]
  • setKey(): since [false, true] is incorrect, setKey() disallows boolean[] and result is reduced to true
  • since no boolean[] are created, duplicate-arguments-array = true does not work

In short:

  • for the user the current behavior of --file remained completely unchanged (incl. default behavior)
  • new: it's now possible to accumulate boolean[] with duplicate-arguments-array = true
  • --file --file or --file=true --file=true or --file true --file true ==> [true, true]

@bcoe
Copy link
Member

@bcoe bcoe commented Jun 22, 2019

@juergba I think this patch is reasonable, if you don't mind completely scrubbing the defaulted behavior, we can go about landing this.

@juergba juergba force-pushed the issue/boolean-default branch from 3aa03f0 to 7a4f9e7 Jun 23, 2019
@bcoe
Copy link
Member

@bcoe bcoe commented Jun 23, 2019

@aorinevo @juergba before I land this, one thing that crossed my mind was this ticket:

yargs/yargs#1334

☝️ which is requesting that we expose defaulted behavior in yargs; is there a better way we could be providing this information upstream?

Should we land this, but come up with a better implementation for defaulted? or should we try to keep the defaulted functionality, but add tests?

@juergba
Copy link
Contributor Author

@juergba juergba commented Jun 24, 2019

Keeping the existing functionality doesn't make sense:

  • it aims only to boolean's
  • it's not working correctly
  • it sets the default at the beginning of the parsing, not at the end.

We would need a better implementation which possibly extends the object returned by detailed() with an additional property eg defaulted. I don't know how this information would be passed upstream to Yargs.

@bcoe bcoe merged commit 17ca3bd into yargs:master Jun 24, 2019
2 checks passed
@juergba juergba deleted the issue/boolean-default branch Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants