-
Notifications
You must be signed in to change notification settings - Fork 123
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 dot notation with coerce #57
Conversation
@maxrimue, I like @nexdrew's approach to explicit arrays: yargs-parser/test/yargs-parser.js Line 2009 in 732acd1
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:
What do you both think? |
@bcoe I'm not fully sure if I get you right, do you mean that inside the 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 Also, this would mean that if I set a coerce function for For example, if I have the option |
@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 |
@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
}) |
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 Based on this, I think we need to drop all coercion from the |
732acd1
to
1bd95e9
Compare
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. |
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.