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.
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.
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.
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.
@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.
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.
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.
I meant specifying defaultValue: 0 instead of letting it default.
There was a problem hiding this comment.
@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.
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.