Skip to content
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

Merged
merged 3 commits into from Oct 14, 2016
Merged

Conversation

ssonal
Copy link
Contributor

@ssonal ssonal commented Oct 11, 2016

Populate argv object with camelcase variants of hyphenated arguments.

Fixes #653

Populate argv object with camelcase variants of hyphenated arguments
Copy link
Member

@bcoe bcoe left a 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)
Copy link
Member

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)
Copy link
Member

@bcoe bcoe Oct 11, 2016

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).

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @ssonal about the order of the logic; otherwise we'd be doing double duty calling coercion functions for both the normal and expanded form of the argument.

I also agree with @bcoe that we should include another test to make sure coercion is applied to both forms.

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.
@bcoe
Copy link
Member

bcoe commented Oct 14, 2016

@ssonal great work: really clean tests, love the feature 👍 look forward to many more contributions from you.

@bcoe bcoe merged commit b1cabae into yargs:master Oct 14, 2016
@ssonal ssonal deleted the develop branch October 14, 2016 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants