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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
474606f
Changed convert logic for map type
krisztianb Mar 27, 2020
294a675
Undo changes of last commit: Changed convert logic for map type
krisztianb Mar 27, 2020
58fb056
No need to convert defaultValue for options of type map
krisztianb Mar 27, 2020
cf16a00
Added test for map declaration option with foreign default value
krisztianb Mar 29, 2020
f9b73a4
Merge branch 'master' of https://github.com/TypeStrong/typedoc
krisztianb Apr 22, 2020
3095a52
Added possible min and max values for a number declaration
krisztianb Apr 24, 2020
52eeb65
Merge branch 'master' of https://github.com/TypeStrong/typedoc
krisztianb Apr 25, 2020
1fa1bd1
Error message creation extracted into a helper function.
krisztianb Apr 25, 2020
95bf83d
Merge branch 'master' of https://github.com/TypeStrong/typedoc
krisztianb Apr 26, 2020
dc33b99
Fixed test for map declaration with foreign default value
krisztianb Apr 26, 2020
1b5fcc8
Merge branch 'master' of https://github.com/TypeStrong/typedoc
krisztianb Apr 28, 2020
52d6e26
Allow every possible number as a defaultValue for a number option
krisztianb May 1, 2020
1b0a71e
Fix indent
krisztianb May 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/lib/utils/options/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

} else {
this._values[declaration.name] = convert(declaration.defaultValue, declaration);
}
Expand Down
41 changes: 2 additions & 39 deletions src/test/utils/options/options.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,53 +41,16 @@ describe('Options', () => {
options.removeDeclarationByName(declaration.name);
});

it('Does not throw if default value is within range for number declaration', () => {
it('Does not throw if default value is out of range for number declaration', () => {
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

@krisztianb krisztianb May 14, 2020 •
edited
Loading

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.

};
options.addDeclaration(declaration);
options.removeDeclarationByName(declaration.name);
});

it('Throws if default value is out of range for number declaration', () => {
const declaration: NumberDeclarationOption = {
name: 'test-number-declaration',
help: '',
type: ParameterType.Number,
minValue: 1,
maxValue: 10,
defaultValue: 0
};
throws(() => options.addDeclaration(declaration));
options.removeDeclarationByName(declaration.name);
});

it('Throws if default value is lower than the min value', () => {
const declaration: NumberDeclarationOption = {
name: 'test-number-declaration',
help: '',
type: ParameterType.Number,
minValue: 1,
defaultValue: 0
};
throws(() => options.addDeclaration(declaration));
options.removeDeclarationByName(declaration.name);
});

it('Throws if default value is greater than the max value', () => {
const declaration: NumberDeclarationOption = {
name: 'test-number-declaration',
help: '',
type: ParameterType.Number,
maxValue: 1,
defaultValue: 2
};
throws(() => options.addDeclaration(declaration));
options.addDeclaration(declaration);
options.removeDeclarationByName(declaration.name);
});

Expand Down