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

feat: add discriminator tag to generate if-then schemas #1376

Merged
merged 5 commits into from Sep 6, 2022

Conversation

daanboer
Copy link
Contributor

Fixes #1209

This pr adds the ability to select a common property in a union type to act as a discriminator. This is done by specifying the desired property key using the discriminator tag. For an example, see the tagged issue or a supplied test case. Note that each of the constituent types should contain this key. A possible next step would be to allow each of the constituent types to define a separate discriminator. The discriminator tag would then not be applied to the union definition, but to the separate object definitions.

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request. I am honestly still a bit suspicious of the utility of this feature but it looks like the code changes are localized so I'm okay with accepting it.

Can you add a test to cover the remaining lines in this patch that are not covered yet? Also, please add an issue for the bug you found.

@@ -15,11 +17,51 @@ export class UnionTypeFormatter implements SubTypeFormatter {
return type instanceof UnionType;
}
public getDefinition(type: UnionType): Definition {
// FIXME: Filtering never types from union types is wrong. However,
Copy link
Member

Choose a reason for hiding this comment

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

Can you file an issue for this? A fixme in the code here is too easy to overlook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was incorrect, filtering never types from unions is valid. However, I thought this statement would be superfluous as never types are filtered upon construction of the union. Apparently, this is not always the case, as the hidden tests point out.

Removed incorrect FIXME.
Removed unreachable code.
@daanboer
Copy link
Contributor Author

daanboer commented Sep 5, 2022

I have removed the line that wasn't tested as it turned out it was unreachable. The FIXME has also been removed, as it was incorrect.

@domoritz domoritz merged commit f7e9cb5 into vega:next Sep 6, 2022
@daanboer daanboer deleted the if-else branch September 9, 2022 10:24
@github-actions
Copy link

🚀 PR was released in v1.1.0 🚀

@github-actions github-actions bot added released This issue/pull request has been released. and removed prerelease labels Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to generate if-then-else schema for union types
2 participants