Skip to content

No need to convert defaultValue for options of type map #1250

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

Merged
merged 4 commits into from
Mar 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 19 additions & 9 deletions src/lib/utils/options/options.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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 = {};
}
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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<DeclarationOption>): void {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice to remove duplicate logic.

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for your specific usage that illustrates why this is important? The one you posted in gitter should be fine, or even something simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Test added. I ran it through. Previously all but the new test passed. Now all tests pass.

this._values[declaration.name] = declaration.defaultValue;
} else {
this._values[declaration.name] = convert(declaration.defaultValue, declaration)
.expect(`Failed to validate default value for ${declaration.name}`);
}
}
}
}

/**
Expand Down
15 changes: 15 additions & 0 deletions src/test/utils/options/options.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down