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 support for Python 3.10 Union syntax #189

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

idoby
Copy link

@idoby idoby commented Jul 14, 2023

Added support for serializing and deserializing Python 3.10 Unions coming from the syntactic sugar |
Also added tests for this case, and all existing tests also pass.

Solves #162

@patrickguenther
Copy link
Contributor

I would have imagined this change to be way more complicated. Does this also work if one or more union constituents have custom deserializers?

@idoby
Copy link
Author

idoby commented Jul 27, 2023

I would have imagined this change to be way more complicated. Does this also work if one or more union constituents have custom deserializers?

I would assume so since all this does is detect Python's different way to represent the union syntax (A|B) and forward it to the rest of the code. So, I believe that if the original union works for the case you describe, this should work as well.

However, it's probably a good idea to add a test to cover what you're suggesting. If you know how to do that quickly, I would appreciate it. I'm not sure I can find the time to delve further into jsons and its testing code.

@patrickguenther
Copy link
Contributor

I see, I think I misunderstood the pull request. Union support was already present and this PR just adds support for the new syntax.

@idoby
Copy link
Author

idoby commented Jul 27, 2023

I see, I think I misunderstood the pull request. Union support was already present and this PR just adds support for the new syntax.

Yes. If Python had chosen to represent the union syntax the same way as the typing.Union object, this code would not be needed at all.

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