-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Automatically port 5.0 System.Text.Json documentation #4284
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
ping @layomia @steveharter @jozkee @dotnet/docs can I get a review? |
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 left a few suggestions that try to address some formatting issues.
Apologies for the delay. I wanted to note earlier that the |
<summary>This class defines how the <see cref="T:System.Text.Json.JsonSerializer" /> deals with references on serialization and deserialization.</summary> | ||
<remarks>To be added.</remarks> | ||
</Docs> | ||
<Members> |
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.
Few things here:
- We recently removed
Default
property, is there something left to do here to avoid conflicts? - We also renamed
ReferenceHandling
toReferenceHandler
; same question as above. - Even though
Default
was removed, I think it is worth to salvage its remarks section and link that to theReferenceHandler == null
behavior.
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.
Any API structure updates will be reflected after the next ref assembly batch gets uploaded to the Docs build. Until then, we can leave these changes here, and then you can address the updates as needed.
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.
@jozkee let's clean this up in a new batch that will include the new dictionary support, field support and others.
Co-authored-by: Tom Dykstra <tdykstra@microsoft.com>
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.
A few comments, but LGTM. Stale APIs can be addressed in a follow up that will include upcoming features. Thanks!
<MemberValue>2</MemberValue> | ||
<Docs> | ||
<summary>To be added.</summary> | ||
<summary>Property will always be serialized and deserialized, regardless of <see cref="P:System.Text.Json.JsonSerializerOptions.IgnoreNullValues" /> configuration.</summary> |
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.
<summary>Property will always be serialized and deserialized, regardless of <see cref="P:System.Text.Json.JsonSerializerOptions.IgnoreNullValues" /> configuration.</summary> | |
<summary>Property will always be serialized and deserialized, regardless of <see cref="P:System.Text.Json.JsonSerializerOptions.DefaultIgnoreCondition" /> configuration.</summary> |
This will need to be changed when the docs for this property are added - https://github.com/dotnet/runtime/blob/master/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs#L239.
<summary>This class defines how the <see cref="T:System.Text.Json.JsonSerializer" /> deals with references on serialization and deserialization.</summary> | ||
<remarks>To be added.</remarks> | ||
</Docs> | ||
<Members> |
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.
@jozkee let's clean this up in a new batch that will include the new dictionary support, field support and others.
@carlossanlop What's the status on this PR? Hopefully we can push it through soon? Do you need help? |
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.
Make list unordered.
Co-authored-by: David Cantu <dacantu@microsoft.com> Co-authored-by: Layomi Akinrinade <layomia@gmail.com>
@steveharter @layomia @jozkee
We want area owners to learn the process of automatically porting new documentation using this tool, and we want people to get used to do this once a month for any new APIs, so they are available in the next Preview.
This PR is to show the commands they can use to run DocsPortingTool, in case they are not yet familiar with them, so you learn how to port documents in your area for the next preview.
Steps:
I used this command first:
.\DocsPortingTool.exe -Docs D:\dotnet-api-docs\xml\ -TripleSlash D:\runtime\artifacts\bin,D:\runtime\artifacts\bin\coreclr\Windows_NT.x64.Release\IL -Save true -IncludedAssemblies System.Text.Json
Merged a commit for what was found in step 1, making sure it all looks ok.
Then I ran this command on top of the already commited changes, to find exceptions:
.\DocsPortingTool.exe -Docs D:\dotnet-api-docs\xml\ -TripleSlash D:\runtime\artifacts\bin,D:\runtime\artifacts\bin\coreclr\Windows_NT.x64.Release\IL -Save true -IncludedAssemblies System.Text.Json -SkipExceptions false
Note: This command is noisy because the tool does not have an easy way to determine if an exception already exists in dotnet-api-docs or not, mainly because tends to change a lot compared to triple slash comments.