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(cli): fix setting flag to false #135

Merged
merged 1 commit into from
Nov 10, 2016
Merged

Conversation

cnishina
Copy link
Member

@cnishina cnishina commented Nov 8, 2016

  • This fixes webdriver-manager update --gecko=false
  • This does not fix webdriver-manager update --gecko=0. Minimist interprets 0 as true.

closes #110

@cnishina
Copy link
Member Author

cnishina commented Nov 8, 2016

Since getValue_ returns 3 different types, the getBoolean check requires a little more verification.

Copy link
Contributor

@sjelin sjelin left a comment

Choose a reason for hiding this comment

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

This has given us a lot of problems so it needs some tests

@@ -31,22 +31,33 @@ export class Option {

getNumber(): number {
let value = this.getValue_();
if (value && typeof value === 'number') {
return +value;
if (value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if value is 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. My fix is for strings or numbers to return the +value. For booleans, I plan to return null.

@cnishina
Copy link
Member Author

cnishina commented Nov 9, 2016

I agree. I'll put tests together.

@cnishina cnishina force-pushed the cli_issues branch 5 times, most recently from cefc610 to 9bebf21 Compare November 9, 2016 23:50
Copy link
Contributor

@sjelin sjelin left a comment

Choose a reason for hiding this comment

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

Just some nits

return Boolean(value);
if (value != null) {
if (typeof value === 'string') {
return !Boolean(value === '0' || value === 'false');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the use of Boolean? The inside is clearly a boolean

if (typeof value === 'string') {
return !Boolean(value === '0' || value === 'false');
} else if (typeof value === 'number') {
return Boolean(value !== 0);
Copy link
Contributor

@sjelin sjelin Nov 9, 2016

Choose a reason for hiding this comment

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

Here too

- This fixes `webdriver-manager update --gecko=false`
- This does not fix `webdriver-manager update --gecko=0`. Minimist interprets 0 as true.
- Add options and programs unit tests

closes angular#110
@cnishina
Copy link
Member Author

Done.

@cnishina cnishina merged commit 5966b6a into angular:master Nov 10, 2016
@cnishina cnishina deleted the cli_issues branch November 10, 2016 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Boolean command line options are broken
3 participants