-
Notifications
You must be signed in to change notification settings - Fork 21
fix: expose ValueJsonConverter for generator support and add JsonSourceGenerator test cases #537
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
Conversation
Signed-off-by: Weihan Li <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #537 +/- ##
=======================================
Coverage 88.48% 88.48%
=======================================
Files 50 50
Lines 2102 2102
Branches 245 245
=======================================
Hits 1860 1860
Misses 191 191
Partials 51 51 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c6821eb to
648d4d5
Compare
Signed-off-by: Weihan Li <[email protected]>
Signed-off-by: Weihan Li <[email protected]>
2ff0d6f to
a82b998
Compare
Signed-off-by: Weihan Li <[email protected]>
Signed-off-by: Weihan Li <[email protected]>
Signed-off-by: Weihan Li <[email protected]>
Signed-off-by: Weihan Li <[email protected]>
Signed-off-by: Weihan Li <[email protected]>
Signed-off-by: Weihan Li <[email protected]>
askpt
left a comment
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.
I don't see any issues with this PR. Does this also sort the #440 or we should investigate it better?
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.
Looks like this does solve #440 as well, at least partially? Should we mention that in the commit or break it out to another?
Looks good to me, let's just be careful to make sure to acknowledge that when we make this public we are adding more surface area to the SDK's API that we must not break; if there's any possible breaking changes we want to make to the serializer, lets make them now.
For this I am happy to keep it in this PR and investigate it better in a separate effort. This PR adds the specific properties to announce this library is NativeAOT, which it seems it is by the test scenario in the pipeline. We can keep it in that PR and then make an official announcement or even add more testing around it.
I agree. I think @kinyoklion and @WeihanLi might have a better answer to it. I cannot see any possible breaking changes to this serialiser in the future (unless we add a new |
|
@askpt @toddbaert thanks very much for the review, it does contribute to the #440 and agree with For the json converter, think we may need to confirm on the convert type name and namespace since it's going to be public, and maybe also the document comment, to avoid future breaking changes. Also cc @kylejuliandev for a review and input. |
This PR
ValueJsonConverterRelated Issues
Fixes #536
Notes
We needed to exclude diagnostics to work around dotnet-format issue, see dotnet/sdk#50012
Follow-up Tasks
How to test