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
Allow every possible number as a defaultValue for a number option #1296
Conversation
@@ -303,6 +303,9 @@ export class Options { | |||
// No need to convert the defaultValue for a map type as it has to be of a specific type | |||
if (declaration.type === ParameterType.Map) { | |||
this._values[declaration.name] = declaration.defaultValue; | |||
} else if (declaration.type === ParameterType.Number) { | |||
// Don't use convert for number options to allow every possible number as a default value | |||
this._values[declaration.name] = declaration.defaultValue || 0; |
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.
So, "every possible number" doesn't include NaN? I think I'm okay with this... will sleep on it.
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.
Yes, I was thinking about that one too. Maybe allowing NaN
is the better solution though. Otherwise I would wonder why the plugin resets it to 0
.
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 disallowing NaN
is a good idea. It is such a common source of bugs that I'd rather just disallow it entirely. If someone really wants an obviously bad value, they can use -Infinity
or 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.
@Gerrit0 Yeah, that's probably the better solution for now. That way you can be sure to have "a number" as a value.
const declaration: NumberDeclarationOption = { | ||
name: 'test-number-declaration', | ||
help: '', | ||
type: ParameterType.Number, | ||
minValue: 1, | ||
maxValue: 10, | ||
defaultValue: 5 |
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.
Let's make the default value explicit so that the test is obvious and doesn't break in the (admittedly very unlikely) situation that the default value is changed.
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.
Sorry, I don't understand. What do you mean by "make it explicit"? Do you mean to create a separate variable for it and use that in the object definition?
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 meant specifying defaultValue: 0
instead of letting it default.
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.
@Gerrit0 Well, it turns out it is already specified just that way: defaultValue: 0
. It's just a confusing display of the diff. 😄
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.
Doh! So it is, diff is great until it isn't. Looks good to me.
Fixes: #1291
PS: Removed some of the tests which are no longer needed.