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

Add typeOptions support for oneOf/Unions #43

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Conversation

ysangkok
Copy link
Contributor

@ysangkok ysangkok commented Sep 18, 2024

We noticed that we couldn't adjust the list of derived classes for union types.
If the union contains a ZonedTime, this is necessary since ZonedTime doesn't
have Eq, which is part of the default deriving list.

This PR adds TypeOptions to untagged unions (CodeGenUnion). It doesn't add
the support for tagged unions, that is left for upcoming work.
It also doesn't fix the format: date-time parsing for inline schemas, which
seems broken, as you can see that the new union in the test cases of this PR
contains a T.Text, which it shouldn't.

oneOf:
- $ref: '#/components/schemas/ZonedTimeType'
- type: string
format: date-time # Bug: This doesn't actually change it from being a Text in the generated code
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah interesting, is format getting ignored here because it's an inlined value instead of a named property?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nevermind, I see you mention that in the pr message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue for this: #44

@OwenGraves
Copy link
Contributor

Would it be fairly straightforward to go ahead and add the same changes to tagged union now as well? It would be nice to have the consistency

@ysangkok ysangkok merged commit cda1a64 into main Sep 18, 2024
1 check passed
@ysangkok ysangkok deleted the type-options-for-unions branch September 18, 2024 16:29
@ysangkok
Copy link
Contributor Author

@OwenGraves It would be nice, but since it's not needed for the current issue we had, I'll just made #45 for it. Really, the whole design around inline schemas is duplicated and fragile and should be refactored. Since everything is potentially in flux in my eyes, I don't wanna be fixing issue we're not actually blocked on.

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

Successfully merging this pull request may close these issues.

2 participants