From 474606f40184207f78f36c02130680fde91ae4b6 Mon Sep 17 00:00:00 2001 From: Krisztian Balla Date: Fri, 27 Mar 2020 21:29:01 +0100 Subject: [PATCH 1/4] 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/4] 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/4] 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/4] 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');