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

UnionConverter: Handle Nested Unions #52

Merged
merged 8 commits into from
May 25, 2020
Merged

UnionConverter: Handle Nested Unions #52

merged 8 commits into from
May 25, 2020

Conversation

bartelink
Copy link
Collaborator

Nested unions don't roundtrip; this PR addresses that by removing the inlining of nested objects when a converter is in play

@bartelink
Copy link
Collaborator Author

bartelink commented May 25, 2020

CC @gardito @fnipo @NickDarvey I can't think of a cleaner way to manage this, thoughts?

@NickDarvey
Copy link

You could maybe check that the JsonConverterAttribute's ConverterType is typeof<UnionConverter> just in case someone has a DU with a field of a record type that has a JsonConverterAttribute. I'm not sure how likely that is though.

@bartelink
Copy link
Collaborator Author

@NickDarvey Hm, that's a damn good question. I'm not sure I know the answer so I did b77261f instead.

I tried to extend the check in the way you suggested, but ran aground and then ran out of time :(

An example case my hack does not address is allowing conversions implemented as a JsonIsomorphism that map the value to a record (I added a IsomorphismUnionEncoder example) to benefit from the inlining without having to use that technique top to bottom.

I'm thinking some mix of having tighter rules in #50 and trying to strip down the impl in #43 may be some of the answer.

For now, I need to ship this so its 🙈for the moment...

Base automatically changed from disco to master May 25, 2020 15:10
@bartelink bartelink changed the title Handle Nested Unions UnionConverter: Handle Nested Unions May 25, 2020
@bartelink bartelink merged commit 2bdcd60 into master May 25, 2020
@bartelink bartelink deleted the nets branch May 25, 2020 15:15
bartelink added a commit that referenced this pull request Dec 10, 2020
@bartelink bartelink mentioned this pull request Dec 10, 2020
4 tasks
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.

3 participants