Skip to content
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

Extend discriminator to support number and boolean values #1729

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

pkuczynski
Copy link

What issue does this pull request resolve?

Part of #1663 - as discussed in the thread...

What changes did you make?

Extending discriminator to support number and boolean types.

Is there anything that requires more attention while reviewing?

I have not changed jtd as I was not even sure if I should or not?

@@ -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})`,
Copy link
Member

Choose a reason for hiding this comment

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

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>
Copy link
Member

Choose a reason for hiding this comment

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

I would also add null.

spec/discriminator.spec.ts Show resolved Hide resolved
throw new Error(`discriminator: "${tagName}" values must be unique strings`)
function addMapping(tagValue: any, i: number): void {
if (
!["string", "number", "boolean"].includes(typeof tagValue) ||
Copy link
Member

Choose a reason for hiding this comment

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

+null

@epoberezkin
Copy link
Member

Great - thank you - made some comments

@Froctnow
Copy link

@pkuczynski can you do review changes? it's useful feature

@fredericosilva
Copy link

fredericosilva commented Oct 26, 2022

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.

@pkuczynski
Copy link
Author

@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?

@pkuczynski
Copy link
Author

@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?

@MxtOUT
Copy link

MxtOUT commented Apr 6, 2023

Hey, any progress on this? I could really use this feature

@pkuczynski
Copy link
Author

@MxtOUT I am still waiting for the feedback from @epoberezkin...

@enzolupia
Copy link

We'd love to get this fix merged, @epoberezkin @pkuczynski any update on this?

@pkuczynski
Copy link
Author

I am waiting for the feedback from @epoberezkin...

@Fire2Bear
Copy link

We would really need this fix on our side, could you have a look please @epoberezkin ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants