Fix dot notation with coerce

[maxrimue]

This PR attempts to fix yargs/yargs#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:

yargs-parser/test/yargs-parser.js

Line 2009 in 732acd1

it('applies coercion function to an explicit array', function () {

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:

yargs.coerce('o', function (obj) {
  return obj.map(function () {}) // some transformation on the object.
})

rather than something like

yargs.coerce('o.banana', function () {})

This would put us in a better position to apply a different transformation for a key such as password:

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:

#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:

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!