-
-
Notifications
You must be signed in to change notification settings - Fork 750
Added possibility to define min and max values for a number option #1278
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
Changes from all commits
474606f
294a675
58fb056
cf16a00
f9b73a4
3095a52
52eeb65
1fa1bd1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,6 +160,16 @@ export interface StringDeclarationOption extends DeclarationOptionBase { | |
| export interface NumberDeclarationOption extends DeclarationOptionBase { | ||
| type: ParameterType.Number; | ||
|
|
||
| /** | ||
| * Lowest possible value. | ||
| */ | ||
| minValue?: number; | ||
|
|
||
| /** | ||
| * Highest possible value. | ||
| */ | ||
| maxValue?: number; | ||
|
|
||
| /** | ||
| * If not specified defaults to 0. | ||
| */ | ||
|
|
@@ -235,9 +245,9 @@ export type DeclarationOptionToOptionType<T extends DeclarationOption> = | |
| * The default conversion function used by the Options container. Readers may | ||
| * re-use this conversion function or implement their own. The arguments reader | ||
| * implements its own since 'false' should not be converted to true for a boolean option. | ||
| * | ||
| * @param value | ||
| * @param option | ||
| * @param value The value to convert. | ||
| * @param option The option for which the value should be converted. | ||
| * @returns The result of the conversion. Might be the value or an error. | ||
| */ | ||
| export function convert<T extends DeclarationOption>(value: unknown, option: T): Result<DeclarationOptionToOptionType<T>, string>; | ||
| export function convert<T extends DeclarationOption>(value: unknown, option: T): Result<unknown, string> { | ||
|
|
@@ -246,7 +256,13 @@ export function convert<T extends DeclarationOption>(value: unknown, option: T): | |
| case ParameterType.String: | ||
| return Result.Ok(value == null ? '' : String(value)); | ||
| case ParameterType.Number: | ||
| return Result.Ok(parseInt(String(value), 10) || 0); | ||
| const numberOption = option as NumberDeclarationOption; | ||
| const numValue = parseInt(String(value), 10) || 0; | ||
| if (hasBounds(numberOption) && | ||
| !valueIsWithinBounds(numValue, numberOption.minValue, numberOption.maxValue)) { | ||
| return Result.Err(getBoundsError(numberOption.name, numberOption.minValue, numberOption.maxValue)); | ||
| } | ||
| return Result.Ok(numValue); | ||
| case ParameterType.Boolean: | ||
| return Result.Ok(Boolean(value)); | ||
| case ParameterType.Array: | ||
|
|
@@ -280,7 +296,13 @@ export function convert<T extends DeclarationOption>(value: unknown, option: T): | |
| } | ||
| } | ||
|
|
||
| function getMapError(map: MapDeclarationOption<unknown>['map'], name: string) { | ||
| /** | ||
| * Returns an error message for a map option, indicating that a given value was not one of the values within the map. | ||
| * @param map The values for the option. | ||
| * @param name The name of the option. | ||
| * @returns The error message. | ||
| */ | ||
| function getMapError(map: MapDeclarationOption<unknown>['map'], name: string): string { | ||
| let keys = map instanceof Map ? [...map.keys()] : Object.keys(map); | ||
| const getString = (key: string) => String(map instanceof Map ? map.get(key) : map[key]); | ||
|
|
||
|
|
@@ -293,3 +315,59 @@ function getMapError(map: MapDeclarationOption<unknown>['map'], name: string) { | |
|
|
||
| return `${name} must be one of ${keys.join(', ')}`; | ||
| } | ||
|
|
||
| /** | ||
| * Returns an error message for a value that is out ot bounds of the given min and/or max values. | ||
| * @param name The name of the thing the value represents. | ||
| * @param minValue The lower bound of the range of allowed values. | ||
| * @param maxValue The upper bound of the range of allowed values. | ||
| * @returns The error message. | ||
| */ | ||
| function getBoundsError(name: string, minValue?: number, maxValue?: number): string { | ||
| if (isFiniteNumber(minValue) && isFiniteNumber(maxValue)) { | ||
| return `${name} must be between ${minValue} and ${maxValue}`; | ||
| } else if (isFiniteNumber(minValue)) { | ||
| return `${name} must be >= ${minValue}`; | ||
| } else if (isFiniteNumber(maxValue)) { | ||
| return `${name} must be <= ${maxValue}`; | ||
| } else { | ||
| return ''; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the given number option has bounds specified. | ||
| * @param numberOption The number option being checked. | ||
| * @retursn True, if the given number option has bounds specified, otherwise false. | ||
| */ | ||
| function hasBounds(numberOption: NumberDeclarationOption): boolean { | ||
| return isFiniteNumber(numberOption.minValue) || isFiniteNumber(numberOption.maxValue); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the given value is a finite number. | ||
| * @param value The value being checked. | ||
| * @returns True, if the value is a finite number, otherwise false. | ||
| */ | ||
| function isFiniteNumber(value?: unknown): value is number { | ||
| return typeof value === 'number' && isFinite(value); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a value is between the bounds of the given min and/or max values. | ||
| * @param value The value being checked. | ||
| * @param minValue The lower bound of the range of allowed values. | ||
| * @param maxValue The upper bound of the range of allowed values. | ||
| * @returns True, if the value is within the given bounds, otherwise false. | ||
| */ | ||
| function valueIsWithinBounds(value: number, minValue?: number, maxValue?: number): boolean { | ||
| if (isFiniteNumber(minValue) && isFiniteNumber(maxValue)) { | ||
| return minValue <= value && value <= maxValue; | ||
| } else if (isFiniteNumber(minValue)) { | ||
| return minValue >= value; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't see this until after I merged, I'll fix it. Should be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry. You're right. I've probably should have written a couple of more tests and this would have popped up. |
||
| } else if (isFiniteNumber(maxValue)) { | ||
| return value <= maxValue; | ||
| } else { | ||
| return true; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import { Logger, Options, ParameterType, ParameterScope } from '../../../lib/utils'; | ||
| import { deepStrictEqual as equal, throws } from 'assert'; | ||
| import { NumberDeclarationOption } from '../../../lib/utils/options'; | ||
| import { deepStrictEqual as equal, doesNotThrow, throws } from 'assert'; | ||
|
|
||
| describe('Options', () => { | ||
| const logger = new Logger(); | ||
|
|
@@ -25,6 +26,43 @@ describe('Options', () => { | |
| options.removeDeclarationByName(declaration.name); | ||
| }); | ||
|
|
||
| it('Does not throw if number declaration has no min and max values', () => { | ||
| const declaration: NumberDeclarationOption = { | ||
| name: 'test-number-declaration', | ||
| help: '', | ||
| type: ParameterType.Number, | ||
| defaultValue: 1 | ||
| }; | ||
| doesNotThrow(() => options.addDeclaration(declaration)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesNotThrow is redundant. All it does is rethrow an error if one occurs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused. What would be the right solution? I guess you understand what it is trying to assert.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| options.removeDeclarationByName(declaration.name); | ||
| }); | ||
|
|
||
| it('Does not throw if default value is within range for number declaration', () => { | ||
| const declaration: NumberDeclarationOption = { | ||
| name: 'test-number-declaration', | ||
| help: '', | ||
| type: ParameterType.Number, | ||
| minValue: 1, | ||
| maxValue: 10, | ||
| defaultValue: 5 | ||
| }; | ||
| doesNotThrow(() => 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('Does not error if a map declaration has a default value that is not part of the map of possible values', () => { | ||
| logger.resetErrors(); | ||
| options.addDeclaration({ | ||
|
|
||
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.
Well this sent me on a wild goose chase. This is equivalent to
Number.isFinite, but the types for that function say they only acceptnumber, 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.