-
Notifications
You must be signed in to change notification settings - Fork 866
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
Update NJsonSchema to v11 #2263
Update NJsonSchema to v11 #2263
Conversation
Noah Stolk seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NoahStolk , LGTM
@NoahStolk , I intend to merge this PR, and then we can figure out a subsequent PR to figure out how best to minimize breaking backward compatibility. Unfortunately I can't merge at the moment as we're doing some internal work to change the protection rules for public repos. |
@NoahStolk , just an FYI that we don't have an ETA for when the protection rules will be fixed (hopefully in a few weeks), but I won't forget about this PR. |
Thanks @rayokota, do you plan to merge this PR as is, and then add the |
If you want to add it to this PR, that would be great. Otherwise we could add it to a subsequent PR |
@NoahStolk , on second thought, can you take a look at adding the directives to this PR? So that we don't break users in case we need to cut a patch release from master. Thanks! |
Will try to find some time to do that this week. |
68efcf9
to
5db4239
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NoahStolk , LGTM
Thanks @rayokota FYI: Not all (.NET 8) unit tests are passing yet, I'm getting some of these errors. Any ideas? If not I'll debug some time later. Haven't had a lot of time to look into this yet. I also don't have .NET 6 installed anymore, so I don't know if the old tests are still passing. I had another question... Do we need to multi-target the |
Thanks @NoahStolk , I believe the .NET 6 tests are still passing. Yeah, I'm not sure about the .NET 8 ones. Let me know if you can't figure it out and maybe I can take a look as well. |
Thanks @NoahStolk. The issue is RicoSuter/NJsonSchema#1696. I'll merge this PR and fix the tests in a subsequent PR. |
Thank you! |
(Original PR: #2185)
Fixes #2169
A few notes:
EnumHandling
no longer exists, so theWithJsonSchemaGeneratorSettingsSerDe
unit test has been updated to useJsonSerializerSettings
instead.