Repository: yargs/yargs-parser
Pull request #57 (2016-08-30 18:38:46)

[WIP] Fix dot notation with coerce
This PR attempts to fix [yargs/yargs#599](https://github.com/yargs/yargs/issues/599).

While the newly added test now passes, two other tests now fail. I haven't figured out yet why they fail, hence this is a WIP PR.

bcoe: @maxrimue, I like @nexdrew's approach to explicit arrays:

https://github.com/yargs/yargs-parser/blob/732acd18e24801b20729029c2f392a43b5a2e0e6/test/yargs-parser.js#L2009

The coerce function gets passed the entire array, which it can then apply a map on. For the sake of consistency, I think it would be neat if:

* coerce got passed the entire object, if an object has been built using dot notation.
* I also think that we should make it so implied arrays: `-f 33 -f 99` get passed as a whole array to the coerce function -- this would be more consistent.

What do you both think?
maxrimue: @bcoe I'm not fully sure if I get you right, do you mean that inside the `setKey` function, it should apply coercions to dot options when the parent option is being processed?

If this is what you mean, I'm not sure if I like that approach too much. The problem I see here is that it might appear weird that dot options get their coercion function's return value not when they are being processed, but when the parent gets processed, even though all of them run through `setKey` once.

Also, this would mean that if I set a coerce function for `foo` and maybe create a dot option `foo.bar`, `bar` would have to cope with `foo`'s coerce function even though people might want different coerce functions.

For example, if I have the option `user` with sub options like `id`, `password` `email` and whatever, chances are high I need to transform values differently for all of them, if at all.
bcoe: @maxrimue I would like @nexdrew's feedback on this too, but it seems more consistent to me if we apply the coercion method to the already built array, or object, rather than key by key -- the nice thing, is you can then lean on built in functions like `Object.map`, `Array.map`, which feels more natural.
bcoe: @maxrimue I would like @nexdrew's feedback on this too, but it seems more consistent to me if we apply the coercion method to the already built array, or object, rather than key by key. This feels fairly elegant:

```js
yargs.coerce('o', function (obj) {
  return obj.map(function () {}) // some transformation on the object.
})
```

rather than something like

```js
yargs.coerce('o.banana', function () {})
```

This would put us in a better position to apply a different transformation for a key such as password:

```js
yargs.coerce('o', function (obj) {
  if (obj.password) obj.password = '[Secret]'
  return obj
})
```

bcoe: @nexdrew @maxrimue mainly I'd like to see us go all one way or the other for `arrays` and `objects`, based on @nexdrew's arguments here:

https://github.com/yargs/yargs-parser/issues/50

This includes updating how the parser handles implicit arrays, what do you think?
nexdrew: I agree that it would be better to coerce the fully-realized object once rather than coerce individual properties separately, mainly because the former is a superset of the latter (i.e. you can still handle individual properties separately when processing the full object, but you can't handle the full object when processing individual values only).

I've been on the fence about implicit arrays, mainly because it seems the author did not intend for the parsed value to be an array, but I've realized the whole point of coercion is to transform the **complete parsed value** into the desired form, so the author can choose how to slice & dice a potential array during coercion (e.g. give precedence to the first or last value) rather than when actually trying to use the value in `argv`. So, all that to say yes, I'm in favor of coercing implicit arrays as an array instead of as individual values.

Based on this, I think we need to drop all coercion from the `setKey` function and, instead, apply coercion as pure post-processing _after all args have been parsed_. So basically change `applyArrayCoercions` into something that applies **all** coercions, to support implicit arrays and args nested within objects.
maxrimue: I tried it again and this time implemented as you suggested. @bcoe, I used this approach as an example for usage you provided:
```javascript
yargs.coerce('o', function (obj) {
  if (obj.password) obj.password = '[Secret]'
  return obj
})
```

You can see in the test that it's similar what I do there. However, I needed to change one other test, you can see that instead of iterating over each value of a passed array, it now expects the entire array to be passed back from the coercion function, which is why the test broke, and I changed it accordingly.

Is this what you had in mind, @bcoe and @nexdrew?
nexdrew: This LGTM! Nice work, @maxrimue! What do you think @bcoe?
bcoe: @maxrimue @nexdrew looks great to me!

Patch:
diff --git a/index.js b/index.js
index e2b35b6..5b092c9 100644
--- a/index.js
+++ b/index.js
@@ -280,7 +280,7 @@ function parse (args, opts) {
   setConfig(argv)
   setConfigObjects()
   applyEnvVars(argv, false)
-  applyArrayCoercions(argv)
+  applyCoercions(argv)
   applyDefaultsAndAliases(argv, flags.aliases, defaults)
 
   // for any counts either not in args or without an explicit default, set to 0
@@ -483,11 +483,9 @@ function parse (args, opts) {
     })
   }
 
-  function applyArrayCoercions (argv) {
+  function applyCoercions (argv) {
     var coerce
-    Object.keys(argv).filter(function (key) {
-      return key === '_' || checkAllAliases(key, flags.arrays)
-    }).forEach(function (key) {
+    Object.keys(argv).forEach(function (key) {
       coerce = checkAllAliases(key, flags.coercions)
       if (typeof coerce === 'function') {
         try {
@@ -538,14 +536,6 @@ function parse (args, opts) {
     })
 
     var key = keys[keys.length - 1]
-    var coerce = !checkAllAliases(key, flags.arrays) && checkAllAliases(key, flags.coercions)
-    if (typeof coerce === 'function') {
-      try {
-        value = coerce(value)
-      } catch (err) {
-        error = err
-      }
-    }
 
     if (value === increment) {
       o[key] = increment(o[key])


Test patch:
diff --git a/test/yargs-parser.js b/test/yargs-parser.js
index 926e750..dac993c 100644
--- a/test/yargs-parser.js
+++ b/test/yargs-parser.js
@@ -1980,11 +1980,25 @@ describe('yargs-parser', function () {
       parsed.f.should.equal(-99)
     })
 
+    it('applies coercion function to all dot options', function () {
+      var parsed = parser(['--foo.bar', 'nananana'], {
+        coerce: {
+          foo: function (val) {
+            val.bar += ', batman!'
+            return val
+          }
+        }
+      })
+      parsed.foo.bar.should.equal('nananana, batman!')
+    })
+
     it('applies coercion function to an implicit array', function () {
       var parsed = parser(['--foo', '99', '-f', '33'], {
         coerce: {
           f: function (arg) {
-            return arg * -1
+            return arg.map(function (a) {
+              return a * -1
+            })
           }
         },
         alias: {