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

Extract annotations into own package #1641 follow up #1642

Closed
wants to merge 2 commits into from

Conversation

trejjam
Copy link
Sponsor Contributor

@trejjam trejjam commented Oct 31, 2023

Move JsonInheritanceAttribute into Annotations package

@trejjam
Copy link
Sponsor Contributor Author

trejjam commented Oct 31, 2023

If you do not find it useful, just close it

@RicoSuter
Copy link
Owner

RicoSuter commented Oct 31, 2023

The problem is that in your model you’d also need the converter in the base class. This would lead to a dep to STJ in annotations.. also newtonsoft would then need the same move into NJS.NewtonsoftJson.Annotations

@RicoSuter
Copy link
Owner

RicoSuter commented Oct 31, 2023

@trejjam
Copy link
Sponsor Contributor Author

trejjam commented Oct 31, 2023

I know that I personally do not need them. I just saw an attribute when I was searching about JsonConvertor<>. And this is the result.

I had in my mind that NJS.NewtonsoftJson.Annotations and NJS.SystemTextJson.Annotations might appear in the future. I do not think I need them now, but I am willing to prepare PR, so they will exist in the stable 11 release.

@trejjam
Copy link
Sponsor Contributor Author

trejjam commented Oct 31, 2023

I did not know that NewtonsoftJson has its own JsonInheritanceAttribute in NJsonSchema.NewtonsoftJson.Converters. It makes this PR useless

@trejjam trejjam closed this Oct 31, 2023
@RicoSuter
Copy link
Owner

The plan is that the default packages will eventually be stj only and newtonsoft lives in additional packges for legacy

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