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

Implement OneOfStructJsonConverter that serializes OneOf structures with value and reference arguments. #4

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

Conversation

r-arjocu
Copy link

@r-arjocu r-arjocu commented Sep 13, 2023

OneOfStructJsonConverter sources with T0-T2 generic arguments and tests for them are provided. Unfortunately I don't know how to write code that generates code (and don't have the time to learn now), but additional implementations with T3 through T8 generic arguments can be easily implemented by copy-paste with minor adjustments and appropriate tests.
The implementation doesn't cover all value types according to Newtonsoft's JTokenType, but these can be relatively easily added with the appropriate tests.

The reasoning behind a second JSON converter in the repo is that I could not make OneOfJsonConverter work correctly directly with OneOf<> structure, because of the latter's restrictions enforced by its design -- I'm certain you're quite aware of them. So I needed a different name for the converter, because of the variant with only T0 generic argument that clashes with OneOfJsonConverter. Do you think that OneOfStructJsonConverter is a good name?

…nd tests, that cover the serialization of OneOf structures with value and reference arguments.
@dpraimeyuu
Copy link
Owner

@ra-mt Sorry for a long time without any answer from my side.

I am totally fine with having a second converter, as long as it gets properly documented by telling the differences and mainly why it exists.

I don't have any "roadmap" so if it helps to solve your problem - I am totally for adding it.

@dpraimeyuu
Copy link
Owner

@ra-mt
What would you say about "namespacing" it by putting classes added by you in a static class:
image?

Alternatively, adding a proper namespace:

  • OneOf.Serialization.Converters.OneOfJsonConverter
  • OneOf.Serialization.Converters.MixedRefsAndStructs.OneOfJsonConverter (it's long but descriptive)

@r-arjocu
Copy link
Author

r-arjocu commented Jan 19, 2024

Unfortunately, I'm changed jobs right now and have other bigger hurdles. I don't expect to be able to contribute for some time; as I see it, I may forget about this. So, my idea is following: you merge what I have provided in this pull request, and from my point of view you can change the code layout (introduce namespaces, static classes etc.) as you deem feet (I won't mind). What do yo say?

I would name it MixedRefsAndValues instead of MixedRefsAndStructs, though, to better describe the intention -- "Refs" and "Values" refer to the OneOf members, whicle "Struct" from OneOfStructJsonConverter refers to OneOf as a struct. Thanks!

@r-arjocu
Copy link
Author

r-arjocu commented Jan 23, 2024

Hi. I will change my username to r-arjocu. I'm unsure how this will effect the pull req.

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.

None yet

2 participants