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

Allow every possible number as a defaultValue for a number option #1296

Merged
merged 13 commits into from May 15, 2020
Merged

Allow every possible number as a defaultValue for a number option #1296

merged 13 commits into from May 15, 2020

Conversation

krisztianb
Copy link
Contributor

Fixes: #1291

PS: Removed some of the tests which are no longer needed.

@@ -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;
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

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. 😄

Copy link
Collaborator

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.

@krisztianb krisztianb requested a review from Gerrit0 May 14, 2020 19:53
@Gerrit0 Gerrit0 merged commit f93c76b into TypeStrong:master May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow every possible number as a defaultValue for a number option
2 participants