From 474606f40184207f78f36c02130680fde91ae4b6 Mon Sep 17 00:00:00 2001 From: Krisztian Balla Date: Fri, 27 Mar 2020 21:29:01 +0100 Subject: [PATCH 1/6] Changed convert logic for map type --- src/lib/utils/options/declaration.ts | 43 +++++++++++----------------- 1 file changed, 16 insertions(+), 27 deletions(-) diff --git a/src/lib/utils/options/declaration.ts b/src/lib/utils/options/declaration.ts index 7b74209c8..6f718cc3f 100644 --- a/src/lib/utils/options/declaration.ts +++ b/src/lib/utils/options/declaration.ts @@ -258,38 +258,27 @@ export function convert(value: unknown, option: T): return Result.Ok([]); case ParameterType.Map: const optionMap = option as MapDeclarationOption; - const key = String(value).toLowerCase(); - if (optionMap.map instanceof Map) { - if (optionMap.map.has(key)) { - return Result.Ok(optionMap.map.get(key)); - } - if ([...optionMap.map.values()].includes(value)) { - return Result.Ok(value); - } - } else { - if (optionMap.map.hasOwnProperty(key)) { - return Result.Ok(optionMap.map[key]); - } - if (Object.values(optionMap.map).includes(value)) { - return Result.Ok(value); - } + if (getTypeOf(value) === getTypeOf(optionMap.defaultValue)) { + return Result.Ok(value); } - return Result.Err(optionMap.mapError ?? getMapError(optionMap.map, optionMap.name)); + return Result.Err(optionMap.mapError ?? `Invalid type for value of ${optionMap.name}`); case ParameterType.Mixed: return Result.Ok(value); } } -function getMapError(map: MapDeclarationOption['map'], name: string) { - let keys = map instanceof Map ? [...map.keys()] : Object.keys(map); - const getString = (key: string) => String(map instanceof Map ? map.get(key) : map[key]); - - // If the map is a TS numeric enum we need to filter out the numeric keys. - // TS numeric enums have the property that every key maps to a value, which maps back to that key. - if (!(map instanceof Map) && keys.every(key => getString(getString(key)) === key)) { - // This works because TS enum keys may not be numeric. - keys = keys.filter(key => Number.isNaN(parseInt(key, 10))); +/** + * Returns the type of a value. + * @param value The value whoes type is wanted. + * @returns The type of the value; + */ +function getTypeOf(value: unknown): unknown { + const typeOfValue = typeof value; + + // null is also of type 'object' + if (typeOfValue === 'object' && value) { + return Object.getPrototypeOf(value) as unknown; + } else { + return typeOfValue; } - - return `${name} must be one of ${keys.join(', ')}`; } From 294a6754de4b9540b7355d128f8338be48d39f89 Mon Sep 17 00:00:00 2001 From: Krisztian Balla Date: Fri, 27 Mar 2020 21:30:14 +0100 Subject: [PATCH 2/6] Undo changes of last commit: Changed convert logic for map type --- src/lib/utils/options/declaration.ts | 43 +++++++++++++++++----------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/src/lib/utils/options/declaration.ts b/src/lib/utils/options/declaration.ts index 6f718cc3f..7b74209c8 100644 --- a/src/lib/utils/options/declaration.ts +++ b/src/lib/utils/options/declaration.ts @@ -258,27 +258,38 @@ export function convert(value: unknown, option: T): return Result.Ok([]); case ParameterType.Map: const optionMap = option as MapDeclarationOption; - if (getTypeOf(value) === getTypeOf(optionMap.defaultValue)) { - return Result.Ok(value); + const key = String(value).toLowerCase(); + if (optionMap.map instanceof Map) { + if (optionMap.map.has(key)) { + return Result.Ok(optionMap.map.get(key)); + } + if ([...optionMap.map.values()].includes(value)) { + return Result.Ok(value); + } + } else { + if (optionMap.map.hasOwnProperty(key)) { + return Result.Ok(optionMap.map[key]); + } + if (Object.values(optionMap.map).includes(value)) { + return Result.Ok(value); + } } - return Result.Err(optionMap.mapError ?? `Invalid type for value of ${optionMap.name}`); + return Result.Err(optionMap.mapError ?? getMapError(optionMap.map, optionMap.name)); case ParameterType.Mixed: return Result.Ok(value); } } -/** - * Returns the type of a value. - * @param value The value whoes type is wanted. - * @returns The type of the value; - */ -function getTypeOf(value: unknown): unknown { - const typeOfValue = typeof value; - - // null is also of type 'object' - if (typeOfValue === 'object' && value) { - return Object.getPrototypeOf(value) as unknown; - } else { - return typeOfValue; +function getMapError(map: MapDeclarationOption['map'], name: string) { + let keys = map instanceof Map ? [...map.keys()] : Object.keys(map); + const getString = (key: string) => String(map instanceof Map ? map.get(key) : map[key]); + + // If the map is a TS numeric enum we need to filter out the numeric keys. + // TS numeric enums have the property that every key maps to a value, which maps back to that key. + if (!(map instanceof Map) && keys.every(key => getString(getString(key)) === key)) { + // This works because TS enum keys may not be numeric. + keys = keys.filter(key => Number.isNaN(parseInt(key, 10))); } + + return `${name} must be one of ${keys.join(', ')}`; } From 58fb056bd4252e88d4586f5f8dafcaf753875c2f Mon Sep 17 00:00:00 2001 From: Krisztian Balla Date: Fri, 27 Mar 2020 21:45:00 +0100 Subject: [PATCH 3/6] No need to convert defaultValue for options of type map --- src/lib/utils/options/options.ts | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/lib/utils/options/options.ts b/src/lib/utils/options/options.ts index 5b679fa13..ef7fbd3e7 100644 --- a/src/lib/utils/options/options.ts +++ b/src/lib/utils/options/options.ts @@ -1,7 +1,7 @@ import * as _ from 'lodash'; import * as ts from 'typescript'; -import { DeclarationOption, ParameterScope, convert, TypeDocOptions, KeyToDeclaration, TypeDocAndTSOptions, TypeDocOptionMap } from './declaration'; +import { DeclarationOption, ParameterScope, ParameterType, convert, TypeDocOptions, KeyToDeclaration, TypeDocAndTSOptions, TypeDocOptionMap } from './declaration'; import { Logger } from '../loggers'; import { Result, Ok, Err } from '../result'; import { insertPrioritySorted } from '../array'; @@ -109,10 +109,7 @@ export class Options { */ reset() { for (const declaration of this._declarations.values()) { - if (declaration.scope !== ParameterScope.TypeScript) { - this._values[declaration.name] = convert(declaration.defaultValue, declaration) - .expect(`Failed to validate default value for ${declaration.name}`); - } + this.setOptionValueToDefault(declaration); } this._compilerOptions = {}; } @@ -169,10 +166,7 @@ export class Options { } } - if (declaration.scope !== ParameterScope.TypeScript) { - this._values[declaration.name] = convert(declaration.defaultValue, declaration) - .expect(`Failed to validate default value for ${declaration.name}`); - } + this.setOptionValueToDefault(declaration); } /** @@ -320,6 +314,22 @@ export class Options { } return errors.length ? Err(errors) : Ok(void 0); } + + /** + * Sets the value of a given option to its default value. + * @param declaration The option whoes value should be reset. + */ + private setOptionValueToDefault(declaration: Readonly): void { + if (declaration.scope !== ParameterScope.TypeScript) { + // No nead 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 { + this._values[declaration.name] = convert(declaration.defaultValue, declaration) + .expect(`Failed to validate default value for ${declaration.name}`); + } + } + } } /** From cf16a0068c5276a397b2c5866324e51f1975f76c Mon Sep 17 00:00:00 2001 From: Krisztian Balla Date: Sun, 29 Mar 2020 21:12:31 +0200 Subject: [PATCH 4/6] Added test for map declaration option with foreign default value --- src/test/utils/options/options.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/test/utils/options/options.test.ts b/src/test/utils/options/options.test.ts index 4ff963e4f..60b678281 100644 --- a/src/test/utils/options/options.test.ts +++ b/src/test/utils/options/options.test.ts @@ -25,6 +25,21 @@ describe('Options', () => { 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({ + name: 'testMapDeclarationWithForeignDefaultValue', + help: '', + type: ParameterType.Map, + map: new Map([ + ['a', 1], + ['b', 2] + ]), + defaultValue: 0 + }); + equal(logger.hasErrors(), false); + }); + it('Supports removing a declaration by name', () => { options.addDeclaration({ name: 'not-an-option', help: '' }); options.removeDeclarationByName('not-an-option'); From 3095a5224e212e42b70844e470822e5d380a5717 Mon Sep 17 00:00:00 2001 From: Krisztian Balla Date: Fri, 24 Apr 2020 21:58:32 +0200 Subject: [PATCH 5/6] Added possible min and max values for a number declaration --- src/lib/utils/options/declaration.ts | 19 +++++++- src/test/utils/options/declaration.test.ts | 50 +++++++++++++++++++++- src/test/utils/options/options.test.ts | 40 ++++++++++++++++- 3 files changed, 106 insertions(+), 3 deletions(-) diff --git a/src/lib/utils/options/declaration.ts b/src/lib/utils/options/declaration.ts index 7b74209c8..9bb99639d 100644 --- a/src/lib/utils/options/declaration.ts +++ b/src/lib/utils/options/declaration.ts @@ -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. */ @@ -246,7 +256,14 @@ export function convert(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 (typeof numberOption.minValue === 'number' && numValue < numberOption.minValue) { + return Result.Err(`${numberOption.name} must be >= ${numberOption.minValue}`); + } else if (typeof numberOption.maxValue === 'number' && numValue > numberOption.maxValue) { + return Result.Err(`${numberOption.name} must be <= ${numberOption.maxValue}`); + } + return Result.Ok(numValue); case ParameterType.Boolean: return Result.Ok(Boolean(value)); case ParameterType.Array: diff --git a/src/test/utils/options/declaration.test.ts b/src/test/utils/options/declaration.test.ts index c154c5542..d9dfff1b7 100644 --- a/src/test/utils/options/declaration.test.ts +++ b/src/test/utils/options/declaration.test.ts @@ -1,4 +1,4 @@ -import { convert, DeclarationOption, ParameterType, MapDeclarationOption } from '../../../lib/utils/options/declaration'; +import { convert, DeclarationOption, ParameterType, MapDeclarationOption, NumberDeclarationOption } from '../../../lib/utils/options/declaration'; import { deepStrictEqual as equal } from 'assert'; import { Result } from '../../../lib/utils'; @@ -16,6 +16,54 @@ describe('Options - Default convert function', () => { equal(convert(NaN, optionWithType(ParameterType.Number)), Result.Ok(0)); }); + it('Converts to number if value is the lowest allowed value for a number option', () => { + const declaration: NumberDeclarationOption = { + name: 'test', + help: '', + type: ParameterType.Number, + minValue: 1, + maxValue: 10, + defaultValue: 1 + }; + equal(convert(1, declaration), Result.Ok(1)); + }); + + it('Generates an error if value is too low for a number option', () => { + const declaration: NumberDeclarationOption = { + name: 'test', + help: '', + type: ParameterType.Number, + minValue: 1, + maxValue: 10, + defaultValue: 1 + }; + equal(convert(0, declaration), Result.Err('test must be >= 1')); + }); + + it('Converts to number if value is the highest allowed value for a number option', () => { + const declaration: NumberDeclarationOption = { + name: 'test', + help: '', + type: ParameterType.Number, + minValue: 1, + maxValue: 10, + defaultValue: 1 + }; + equal(convert(10, declaration), Result.Ok(10)); + }); + + it('Generates an error if value is too high for a number option', () => { + const declaration: NumberDeclarationOption = { + name: 'test', + help: '', + type: ParameterType.Number, + minValue: 1, + maxValue: 10, + defaultValue: 1 + }; + equal(convert(11, declaration), Result.Err('test must be <= 10')); + }); + it('Converts to strings', () => { equal(convert('123', optionWithType(ParameterType.String)), Result.Ok('123')); equal(convert(123, optionWithType(ParameterType.String)), Result.Ok('123')); diff --git a/src/test/utils/options/options.test.ts b/src/test/utils/options/options.test.ts index 60b678281..07d70582f 100644 --- a/src/test/utils/options/options.test.ts +++ b/src/test/utils/options/options.test.ts @@ -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)); + 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({ From 1fa1bd15ca9d743393d978fb5631764547fc4ca0 Mon Sep 17 00:00:00 2001 From: Krisztian Balla Date: Sat, 25 Apr 2020 23:29:51 +0200 Subject: [PATCH 6/6] Error message creation extracted into a helper function. --- src/lib/utils/options/declaration.ts | 77 +++++++++++++++++++--- src/test/utils/options/declaration.test.ts | 4 +- 2 files changed, 71 insertions(+), 10 deletions(-) diff --git a/src/lib/utils/options/declaration.ts b/src/lib/utils/options/declaration.ts index 9bb99639d..cbfced864 100644 --- a/src/lib/utils/options/declaration.ts +++ b/src/lib/utils/options/declaration.ts @@ -245,9 +245,9 @@ export type DeclarationOptionToOptionType = * 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(value: unknown, option: T): Result, string>; export function convert(value: unknown, option: T): Result { @@ -258,10 +258,9 @@ export function convert(value: unknown, option: T): case ParameterType.Number: 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}`); - } else if (typeof numberOption.maxValue === 'number' && numValue > numberOption.maxValue) { - return Result.Err(`${numberOption.name} must be <= ${numberOption.maxValue}`); + 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: @@ -297,7 +296,13 @@ export function convert(value: unknown, option: T): } } -function getMapError(map: MapDeclarationOption['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['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]); @@ -310,3 +315,59 @@ function getMapError(map: MapDeclarationOption['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; + } else if (isFiniteNumber(maxValue)) { + return value <= maxValue; + } else { + return true; + } +} diff --git a/src/test/utils/options/declaration.test.ts b/src/test/utils/options/declaration.test.ts index d9dfff1b7..0975cb1d6 100644 --- a/src/test/utils/options/declaration.test.ts +++ b/src/test/utils/options/declaration.test.ts @@ -37,7 +37,7 @@ describe('Options - Default convert function', () => { maxValue: 10, defaultValue: 1 }; - equal(convert(0, declaration), Result.Err('test must be >= 1')); + equal(convert(0, declaration), Result.Err('test must be between 1 and 10')); }); it('Converts to number if value is the highest allowed value for a number option', () => { @@ -61,7 +61,7 @@ describe('Options - Default convert function', () => { maxValue: 10, defaultValue: 1 }; - equal(convert(11, declaration), Result.Err('test must be <= 10')); + equal(convert(11, declaration), Result.Err('test must be between 1 and 10')); }); it('Converts to strings', () => {