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

Port UnionConverter to STJ re #43 #59

Merged
merged 67 commits into from
Jan 4, 2022
Merged

Conversation

bartelink
Copy link
Collaborator

@bartelink bartelink commented Dec 9, 2020

The bulk of the commits in this PR derive from #43 by @NickDarvey

In particular, the entire implementation and test suite was already complete, but was parked (and inadvertently closed, hence its in this PR).

The later commits effect minor cleanups:

  • uses [ugly] #ifdefs to make clear the commonalities (and, most importantly, the minor differences) between the System.Text.Json and Newtonsoft.Json implementations wrt Pickling and JsonIsomorphism
  • unifying the UnionConverterTests to test the STJ and JNK variants with a common test suite
  • cherry-picking nested unions support from UnionConverter: Handle Nested Unions #52
  • fix 3 failing tests signifying incomplete handling of nested unions from ☝️

ylibrach and others added 30 commits March 5, 2020 12:24
* Replaces Newtonsoft bits from scaffold with the actual STJ bits
* Tries to retain symmetry with Newtonsoft UnionConverter impl
(Oh, they're all failing)
* Wrong namespace lol, and
* It seems like boxing a DU means that STJ won't use the convert for serialization. Maybe this is something to do with derived types and JsonConverter dotnet/runtime#30427
@bartelink bartelink force-pushed the nickdarvey-unionconverter-stj branch from 69ca5fc to 2481fb6 Compare January 3, 2022 10:27
@bartelink bartelink marked this pull request as ready for review January 3, 2022 10:31
@bartelink
Copy link
Collaborator Author

Woohoo I found the problem/solution (some tidying and/or extending of test suite still to be done, but seems this is on track for finally being released)
cc @NickDarvey

@bartelink bartelink removed the help wanted Extra attention is needed label Jan 3, 2022
@bartelink bartelink changed the title Port UnionConverter to STJ Port UnionConverter to STJ re #43 Jan 4, 2022
@bartelink bartelink merged commit 491e84b into master Jan 4, 2022
@bartelink bartelink deleted the nickdarvey-unionconverter-stj branch January 4, 2022 11:23
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