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.

Sign up for GitHub

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

Jump to bottom

Update yargs to 6.5.0 #360

Merged
merged 3 commits into from
Feb 13, 2017
Merged

Update yargs to 6.5.0 #360

merged 3 commits into from
Feb 13, 2017

Conversation

max-mironov
Copy link
Contributor

This should fix issue #339 "Can not perform rollback if deployment name is same as CodePush CLI command #339"

Update yargs to 6.5.0
5c4d8dc 
This shpuld fix issue microsoft#339 "Can not perform rollback if deployment name is same as CodePush CLI command microsoft#339"
@msftclas
Copy link

Hi @max-mironov, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

@max-mironov max-mironov mentioned this pull request Dec 30, 2016
Closed
richardhuaaa
richardhuaaa suggested changes Feb 9, 2017
View reviewed changes
Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

Could you explain a bit more how this PR fixes the issue? Why did we change all of the demand() calls to require fewer arguments, and are we sure that there are no breaking changes (we are upgrading across 3 major versions here, so it's quite a big jump)?

@max-mironov
Copy link
Contributor Author

@Silhouettes The issue described in #339 was fixed in yargs 4 and upper versions. I've looked at change logs at https://github.com/yargs/yargs/blob/v6.5.0/CHANGELOG.md#v400-2016214-127-0700 but unfortunately was unable to find the exact notice about the described issue (I think may be fixed in this PR yargs/yargs#368). So I've only determined that versions below 4.0 have the same problem;)

The most part of changes in API were introduced in 4.0 version but I suppose that if it is needed to test this changes anyway it would be better to use the latest version because they might have fixed any other issues as well.
So the API methods were slightly changed. For now demand() points to the number of arguments followed by the command e.g.
code-push release <appName> <updateContentsPath> <targetBinaryVersion> requires 3 non-option arguments followed by the 'release' command, so we changed this to demand(3, 3) and etc.
I've tried to do my best to verify that all CLI commands are running as expected with updated yargs. But we can double check it again if needed,

richardhuaaa
richardhuaaa approved these changes Feb 9, 2017
View reviewed changes
Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation @max-mironov, that makes sense! I just left 3 small comments about an undocumented parameter used for testing. If you can address those, and you are confident there are no other breaking changes that will affect us, then feel free to merge :)

cli/script/command-parser.ts Outdated
@@ -316,7 +316,7 @@ var argv = yargs.usage(USAGE_PREFIX + " <command>")
isValidCommandCategory = true;
isValidCommand = true;
yargs.usage(USAGE_PREFIX + " login [options]")
.demand(/*count*/ 1, /*max*/ 2) // Require one non-optional and one optional argument.
.demand(/*count*/ 0, /*max*/ 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still want max to be one here. We have an undocumented argument that can be used for testing, eg:

code-push login http://localhost:3000

Copy link
Contributor

Choose a reason for hiding this comment

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

While you're at it, could you please leave a (short) comment explaining that, here and in the other places?

cli/script/command-parser.ts Outdated
@@ -306,7 +306,7 @@ var argv = yargs.usage(USAGE_PREFIX + " <command>")
isValidCommandCategory = true;
isValidCommand = true;
yargs.usage(USAGE_PREFIX + " link")
.demand(/*count*/ 1, /*max*/ 2) // Require one non-optional and one optional argument.
.demand(/*count*/ 0, /*max*/ 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as for login command, I think max should be one because we have an undocumented option used for testing

cli/script/command-parser.ts Outdated
@@ -369,7 +369,7 @@ var argv = yargs.usage(USAGE_PREFIX + " <command>")
isValidCommandCategory = true;
isValidCommand = true;
yargs.usage(USAGE_PREFIX + " register")
.demand(/*count*/ 1, /*max*/ 2) // Require one non-optional and one optional argument.
.demand(/*count*/ 0, /*max*/ 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as for login/link commands

Update yargs to 6.5.0
698d6f4 
Fixed issue when it was not allowed to use additional undocument parameter for some commands
@max-mironov
Copy link
Contributor Author

@Silhouettes , thanks for pointing at this, I've pushed required changes. I'd like to perform some little more testing of this and I'm going to merge this changes next week.

@max-mironov max-mironov merged commit 25f4fb8 into microsoft:master Feb 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardhuaaa richardhuaaa richardhuaaa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@max-mironov @msftclas @richardhuaaa