Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
JSON support required properties #73063
JSON support required properties #73063
Changes from all commits
c69825f
7852d75
caa063c
44fe21b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we wish to set other applicable properties directly on
JsonPropertyInfo
rather than the intermediateJsonPropertyInfoValues
(e.g.IsExtensionData
)?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.
Generally speaking we could probably get rid of most of the usages of JsonMetadataServices but that might be a bit more work since we'd have to refactor also JsonTypeInfo part. While I could start doing it gradually it probably makes more sense to do it in one go but for that we need to be able to customize parameters which currently contract customization doesn't support. The reason this one is separate is because it doesn't have API in JsonMetadataServices but because we can avoid adding such API I went ahead and did it this way. I'll keep changes in this PR to minimum, we can revisit this once we get full parameters support
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.
Can't the default constructor be removed here? Or is it required by ApiCompat?
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.
it's not, I followed existing pattern with JsonInclude. We can put better xml doc comment I guess