Extend discriminator to support number and boolean values #1729
Conversation
| @@ -32,7 +32,7 @@ const def: CodeKeywordDefinition = { | |||
| const valid = gen.let("valid", false) | |||
| const tag = gen.const("tag", _`${data}${getProperty(tagName)}`) | |||
| gen.if( | |||
| _`typeof ${tag} == "string"`, | |||
| _`["string", "number", "boolean"].includes(typeof ${tag})`, | |||
There was a problem hiding this comment.
This can be improved. The compilation phase should collect all tags and in case they are all the same type, this type would be used. In general case it should use something like or(...tagTypes.map((t) => _`typeof ${tag} == ${t}`)) - include call is inefficient. This general case is likely to be ok for tagTypes with a single item, if not - or function can be improved (but I think it should just work).
| @@ -57,8 +57,10 @@ const def: CodeKeywordDefinition = { | |||
| return _valid | |||
| } | |||
|
|
|||
| function getMapping(): {[T in string]?: number} { | |||
| const oneOfMapping: {[T in string]?: number} = {} | |||
| type OneOfMapping = Map<string | number | boolean, number> | |||
| throw new Error(`discriminator: "${tagName}" values must be unique strings`) | ||
| function addMapping(tagValue: any, i: number): void { | ||
| if ( | ||
| !["string", "number", "boolean"].includes(typeof tagValue) || |
|
Great - thank you - made some comments |
|
@pkuczynski can you do review changes? it's useful feature |
|
I'd also like to use this feature. @pkuczynski let me know if you don't have time for it, I can take it form here. |
|
@Froctnow @fredericosilva I just pushed changes requested by @epoberezkin. @epoberezkin can you have a look again and resolve threads if you are happy with my changes? |
|
@epoberezkin I am not sure where did the build failure came from, as I haven't touched those files. Can you help me to solve it? |
|
Hey, any progress on this? I could really use this feature |
|
@MxtOUT I am still waiting for the feedback from @epoberezkin... |
|
We'd love to get this fix merged, @epoberezkin @pkuczynski any update on this? |
|
I am waiting for the feedback from @epoberezkin... |
|
We would really need this fix on our side, could you have a look please @epoberezkin ? |
What issue does this pull request resolve?
Part of #1663 - as discussed in the thread...
What changes did you make?
Extending
discriminatorto supportnumberandbooleantypes.Is there anything that requires more attention while reviewing?
I have not changed
jtdas I was not even sure if I should or not?