No need to convert defaultValue for options of type map #1250
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gerrit0
requested changes
Mar 29, 2020
| private setOptionValueToDefault(declaration: Readonly<DeclarationOption>): 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good idea. Test added. I ran it through. Previously all but the new test passed. Now all tests pass.
| * 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 { |
There was a problem hiding this comment.
👍 nice to remove duplicate logic.
|
Thanks! I'm not sure if I'll get a release out today, next Saturday at the latest. Going to be busy... |
|
No hurry on this one. 😄 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #1249
The convert function was used to "validate" the default value for all options. This is not necessary for options of type map, since the type of the default value is provided via the template class MapDeclarationOption and must be of the right type. Moreover the conversion throws an error if the default value for the option is of the right type, but not listed in the map of possible values.