-
Notifications
You must be signed in to change notification settings - Fork 991
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
feat(command): add camelcase commands to argv #658
Conversation
Populate argv object with camelcase variants of hyphenated arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really dig this implementation! just a few minor comments.
@@ -185,6 +186,11 @@ module.exports = function (yargs, usage, validation) { | |||
if (demand.variadic) argv[demand.cmd] = argv._.splice(0) | |||
else argv[demand.cmd] = argv._.shift() | |||
postProcessPositional(yargs, argv, demand.cmd) | |||
if (/-/.test(demand.cmd)) { | |||
var cc = camelCase(demand.cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be worthwhile pulling this into a helper function, e.g., addCamelcaseExpansions ()
? since the logic is the same in optional and required arguments.
@@ -194,6 +200,11 @@ module.exports = function (yargs, usage, validation) { | |||
if (maybe.variadic) argv[maybe.cmd] = argv._.splice(0) | |||
else argv[maybe.cmd] = argv._.shift() | |||
postProcessPositional(yargs, argv, maybe.cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also apply postProcessPositional()
to both the normal form, and expanded form of the argument; perhaps we could make postProcessPositional()
take an array? and addCamelcaseExpansions ()
could return an array of either: the original argument, or the original argument plus the camel-case expansion?
We'd need to add one more test, verifying that coerce
is applied to the camel-case version of the key (added along-side the existing coerce test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this redundant though?
This is how I've understood things:
Since coerce
acts on the value argv[key]
, irrespective of whether the key is the original hyphenated argument or the newly generated camel-case variant, argv[key]
should hold the same value. Additionally since coerce
isn't provided the key itself, the value it returns isn't affected by the camelCase argument.
That is why I copied values over to the camel-case arguments after postProcessPositional()
.
Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test hyphenated variadric arguments to ensure that camel-case keys are defined and hold the right values. Test coerce() to ensure coercing hyphenated arguments also applies this result to camel-case values.
@ssonal great work: really clean tests, love the feature 👍 look forward to many more contributions from you. |
Populate argv object with camelcase variants of hyphenated arguments.
Fixes #653