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

Fix: WCT CLI arg standardizing #424

Merged
merged 1 commit into from
Jun 18, 2018
Merged

Conversation

stramel
Copy link
Contributor

@stramel stramel commented May 31, 2018

@merlinnot
Copy link
Contributor

Breaking change btw ;)

@stramel
Copy link
Contributor Author

stramel commented Jun 1, 2018

Or is it a fix since it should have been this way in the first place? 🤔 😛

Copy link
Contributor

@usergenic usergenic left a comment

Choose a reason for hiding this comment

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

Okay. I'll need to make the changelog item more explicit in case this does break somebody's use-case.

@usergenic
Copy link
Contributor

Copy link
Contributor

@usergenic usergenic left a comment

Choose a reason for hiding this comment

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

I'm going to need to hold off on merging this one for now unless we can also extend the code to support the old form, since it is technically a breaking change and there are uses in-the-wild.

@stramel
Copy link
Contributor Author

stramel commented Jun 13, 2018

@usergenic I wonder if I could set alias: 'configFile'?

@usergenic
Copy link
Contributor

@stramel that occurred to me too. haven't tested it but thats probably the way to go.

@stramel
Copy link
Contributor Author

stramel commented Jun 13, 2018

Yeah, I won't be able to test until I get home. But if it works I will update the PR and add a note

@merlinnot
Copy link
Contributor

Maybe a deprecation warning on top of this? So we can actually drop it some day?

@stramel
Copy link
Contributor Author

stramel commented Jun 14, 2018

FYI: abbr or alias can only be a single character... I will look into a different way to resolve that issue.

@stramel
Copy link
Contributor Author

stramel commented Jun 14, 2018

@usergenic Perhaps this is just a happy coincidence but this isn't a breaking change, oddly enough. It is "breaking" in the sense that we are deprecating the configFile and simpleOutput option. However, nomnom the arg parser that is used in web-component-tester will match configFile and config-file after these changes. Where as previously it did not match config-file as configFile. See output below:

# Works with previous camelCase arg
$ ./packages/web-component-tester/bin/wct --configFile=polymer.test.json
args [ '--configFile=polymer.test.json' ]
options { configFile: 'polymer.test.json' }

# Works with added dash-case arg
$ ./packages/web-component-tester/bin/wct --config-file=polymer.test.json
args [ '--config-file=polymer.test.json' ]
options { configFile: 'polymer.test.json' }

Outputting the context.options after parsing appears to have the proper file set in both cases as well.

...
configFile: 'polymer.test.json',
...

Technically speaking any arg listed camelCase in the definition list will work camelCase even with full specified as dash-case.

NOTE: nomnom is deprecated and would probably be useful to move to command-line-args like the other packages on the next major breaking change release and coordinate that with the remove of camelCase arg support.

NOTE: cli should probably switch simpleOutput to be simple-output as well but that WILL be a breaking change since command-line-args does NOT treat args the same way as nomnom. (Heads up @krumware)

@usergenic
Copy link
Contributor

@stramel whoa! how about that! okay, that's great work looking into that. After we merge this I'll add a test or something to ensure handling of both forms in the event someone decides to upgrade wct's arg parsing without knowing all this.

@usergenic
Copy link
Contributor

I'll get this merged before tomorrow as soon as I can set aside a moment to craft that test.

@usergenic
Copy link
Contributor

Had some other issues with CLI release branch on Friday; Will get this merged today (if possible!)

@usergenic usergenic merged commit 1f6216d into Polymer:master Jun 18, 2018
@stramel stramel deleted the ms/fix-cli-options branch June 18, 2018 21:45
@stramel
Copy link
Contributor Author

stramel commented Jun 18, 2018

No worries! Thanks again @usergenic!

@usergenic
Copy link
Contributor

@stramel this is getting released shortly after #515 gets approved. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants