Added possibility to define min and max values for a number option #1278
Conversation
| type: ParameterType.Number, | ||
| defaultValue: 1 | ||
| }; | ||
| doesNotThrow(() => options.addDeclaration(declaration)); |
There was a problem hiding this comment.
doesNotThrow is redundant. All it does is rethrow an error if one occurs.
There was a problem hiding this comment.
I'm confused. What would be the right solution? I guess you understand what it is trying to assert.
There was a problem hiding this comment.
It doesn't hurt anything, I guess there is some semantic value, but that's provided by the test name. It's fine as is.
src/lib/utils/options/declaration.ts
Outdated
| const numberOption = option as NumberDeclarationOption; | ||
| const numValue = parseInt(String(value), 10) || 0; | ||
| if (typeof numberOption.minValue === 'number' && numValue < numberOption.minValue) { | ||
| return Result.Err(`${numberOption.name} must be >= ${numberOption.minValue}`); |
There was a problem hiding this comment.
Is there a reason you don't include both bounds in the error message? It seems like it would be more useful:
$ typedoc --numOption 10
Error: numOption must be <= 5
$ typedoc --numOption 2
Error: numOption must be >= 3
# vs
$ typedoc --numOption 10
Error: numOption must be between 2 and 10Probably best to extract the error message creation to a helper function, since there are several possible combinations.
There was a problem hiding this comment.
I agree that if both are defined it would be more useful to include both bounds in the error message.
| * @returns True, if the value is a finite number, otherwise false. | ||
| */ | ||
| function isFiniteNumber(value?: unknown): value is number { | ||
| return typeof value === 'number' && isFinite(value); |
There was a problem hiding this comment.
Well this sent me on a wild goose chase. This is equivalent to Number.isFinite, but the types for that function say they only accept number, so TS would yell... this was fixed, but the fix changed generated files instead of the source files and so was thrown out in the next release.
| type: ParameterType.Number, | ||
| defaultValue: 1 | ||
| }; | ||
| doesNotThrow(() => options.addDeclaration(declaration)); |
There was a problem hiding this comment.
It doesn't hurt anything, I guess there is some semantic value, but that's provided by the test name. It's fine as is.
| if (isFiniteNumber(minValue) && isFiniteNumber(maxValue)) { | ||
| return minValue <= value && value <= maxValue; | ||
| } else if (isFiniteNumber(minValue)) { | ||
| return minValue >= value; |
There was a problem hiding this comment.
Didn't see this until after I merged, I'll fix it. Should be minValue <= value
There was a problem hiding this comment.
Sorry. You're right. I've probably should have written a couple of more tests and this would have popped up.
Also added a couple of tests for the new feature.