Repository: yargs/yargs-parser
Pull request #184 (2019-06-15 07:15:42)

Fix "boolean" flag with "duplicate-arguments-array"
### Description

```js
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


aorinevo: 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 be 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 when set.  

aorinevo: @juergba, what happens when you pass ‘—file=true —file=true’?
juergba: @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: 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: 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:
- 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: @juergba I think this patch is reasonable, if you don't mind completely scrubbing the `defaulted` behavior, we can go about landing this.
bcoe: @aorinevo @juergba before I land this, one thing that crossed my mind was this ticket:

https://github.com/yargs/yargs/issues/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: 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.

Patch:
diff --git a/index.js b/index.js
index 315d1aa..4fd0553 100644
--- a/index.js
+++ b/index.js
@@ -46,7 +46,6 @@ function parse (args, opts) {
     counts: {},
     normalize: {},
     configs: {},
-    defaulted: {},
     nargs: {},
     coercions: {},
     keys: []
@@ -132,14 +131,6 @@ function parse (args, opts) {
   })
 
   var argv = { _: [] }
-
-  Object.keys(flags.bools).forEach(function (key) {
-    if (Object.prototype.hasOwnProperty.call(defaults, key)) {
-      setArg(key, defaults[key])
-      setDefaulted(key)
-    }
-  })
-
   var notFlags = []
 
   for (var i = 0; i < args.length; i++) {
@@ -406,8 +397,6 @@ function parse (args, opts) {
   }
 
   function setArg (key, val) {
-    unsetDefaulted(key)
-
     if (/-/.test(key) && configuration['camel-case-expansion']) {
       var alias = key.split('.').map(function (prop) {
         return camelCase(prop)
@@ -560,7 +549,7 @@ function parse (args, opts) {
       } else {
         // setting arguments via CLI takes precedence over
         // values within the config file.
-        if (!hasKey(argv, fullKey.split('.')) || (flags.defaulted[fullKey]) || (flags.arrays[fullKey] && configuration['combine-arrays'])) {
+        if (!hasKey(argv, fullKey.split('.')) || (flags.arrays[fullKey] && configuration['combine-arrays'])) {
           setArg(fullKey, value)
         }
       }
@@ -589,7 +578,7 @@ function parse (args, opts) {
           return camelCase(key)
         })
 
-        if (((configOnly && flags.configs[keys.join('.')]) || !configOnly) && (!hasKey(argv, keys) || flags.defaulted[keys.join('.')])) {
+        if (((configOnly && flags.configs[keys.join('.')]) || !configOnly) && !hasKey(argv, keys)) {
           setArg(keys.join('.'), process.env[envVar])
         }
       }
@@ -704,7 +693,7 @@ function parse (args, opts) {
       }
     } else if (o[key] === undefined && isTypeArray) {
       o[key] = isValueArray ? value : [value]
-    } else if (duplicate && !(o[key] === undefined || checkAllAliases(key, flags.bools) || checkAllAliases(keys.join('.'), flags.bools) || checkAllAliases(key, flags.counts))) {
+    } else if (duplicate && !(o[key] === undefined || checkAllAliases(key, flags.counts))) {
       o[key] = [ o[key], value ]
     } else {
       o[key] = value
@@ -762,18 +751,6 @@ function parse (args, opts) {
     return isSet
   }
 
-  function setDefaulted (key) {
-    [].concat(flags.aliases[key] || [], key).forEach(function (k) {
-      flags.defaulted[k] = true
-    })
-  }
-
-  function unsetDefaulted (key) {
-    [].concat(flags.aliases[key] || [], key).forEach(function (k) {
-      delete flags.defaulted[k]
-    })
-  }
-
   // make a best effor to pick a default value
   // for an option based on name and type.
   function defaultValue (key) {


Test patch:
diff --git a/test/yargs-parser.js b/test/yargs-parser.js
index c908187..abcfaaf 100644
--- a/test/yargs-parser.js
+++ b/test/yargs-parser.js
@@ -1108,7 +1108,7 @@ describe('yargs-parser', function () {
         })
       })
 
-      describe('with implied false default', function () {
+      describe('without any default value', function () {
         var opts = null
 
         beforeEach(function () {
@@ -1125,8 +1125,8 @@ describe('yargs-parser', function () {
           parser(['--no-flag'], opts).flag.should.be.false // eslint-disable-line
         })
 
-        it('should set false if no flag in arg', function () {
-          expect(parser([], opts).flag).to.be.undefined // eslint-disable-line
+        it('should not add property if no flag in arg', function () {
+          parser([''], opts).should.not.have.property('flag')
         })
       })
 
@@ -2334,6 +2334,18 @@ describe('yargs-parser', function () {
             parsed['x'].should.deep.equal(3)
           })
         })
+        describe('type=boolean', function () {
+          it('[-x true -x true -x false] => false', function () {
+            var parsed = parser('-x true -x true -x false', {
+              boolean: ['x'],
+              configuration: {
+                'duplicate-arguments-array': false,
+                'flatten-duplicate-arrays': false
+              }
+            })
+            parsed['x'].should.deep.equal(false)
+          })
+        })
       })
       describe('duplicate=false, flatten=true,', function () {
         describe('type=array', function () {
@@ -2370,6 +2382,18 @@ describe('yargs-parser', function () {
             parsed['x'].should.deep.equal(3)
           })
         })
+        describe('type=boolean', function () {
+          it('[-x true -x true -x false] => false', function () {
+            var parsed = parser('-x true -x true -x false', {
+              boolean: ['x'],
+              configuration: {
+                'duplicate-arguments-array': false,
+                'flatten-duplicate-arrays': true
+              }
+            })
+            parsed['x'].should.deep.equal(false)
+          })
+        })
       })
       describe('duplicate=true, flatten=true,', function () {
         describe('type=array', function () {
@@ -2406,6 +2430,18 @@ describe('yargs-parser', function () {
             parsed['x'].should.deep.equal([1, 2, 3])
           })
         })
+        describe('type=boolean', function () {
+          it('[-x true -x true -x false] => [true, true, false]', function () {
+            var parsed = parser('-x true -x true -x false', {
+              boolean: ['x'],
+              configuration: {
+                'duplicate-arguments-array': true,
+                'flatten-duplicate-arrays': true
+              }
+            })
+            parsed['x'].should.deep.equal([true, true, false])
+          })
+        })
       })
       describe('duplicate=true, flatten=false,', function () {
         describe('type=array', function () {
@@ -2442,6 +2478,18 @@ describe('yargs-parser', function () {
             parsed['x'].should.deep.equal([1, 2, 3])
           })
         })
+        describe('type=boolean', function () {
+          it('[-x true -x true -x false] => [true, true, false]', function () {
+            var parsed = parser('-x true -x true -x false', {
+              boolean: ['x'],
+              configuration: {
+                'duplicate-arguments-array': true,
+                'flatten-duplicate-arrays': false
+              }
+            })
+            parsed['x'].should.deep.equal([true, true, false])
+          })
+        })
       })
     })