-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Automatically port System.Text.Json triple slash comments #2737
Conversation
@rpetrusha do you mind checking this PR next? |
@ahsonkhan @steveharter please help review these ported comments. |
I'm checking it now, @carlossanlop. |
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've started making changes up to Utf8JsonReader, @carlossanlop. I'll finish tomorrow. Please review what I've finished so far. Tomorrow, I'll finish and approve, you can review, and I'll merge.
Co-Authored-By: Ron Petrusha <ronpet@microsoft.com>
Co-Authored-By: Ahson Khan <ahkha@microsoft.com>
@ahsonkhan there are multiple complex requests that should be addressed separately due to recent changes in source. Let's get this PR merged first, then update the documentation in a new PR. @rpetrusha @mairaw I addressed the last comments. Can you please take a last look to see if we can get it merged? |
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.
This looks good, @carlossanlop, except that the build report showed 7 broken crefs. I think I've fixed them all, so I'll merge when a successful build completes.
@thanks @rpetrusha. I fixed additional JSON literals that needed to be changed from |
A bunch of APIs changed between preview6/preview7. @rpetrusha - thanks for fixing some of the links. |
I think so, @ahsonkhan. We've updated the API reference to reflect Preview 7 APIs, and the build report isn't showing any additional broken links. |
@@ -777,9 +789,8 @@ This method does not parse the contents of a JSON string value. | |||
<summary>Gets the value at a specified index when the current value is an <see cref="F:System.Text.Json.JsonValueKind.Array" />.</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.
Looks like we are missing the docs from source for the the other property, ValueKind:
dotnet-api-docs/xml/System.Text.Json/JsonElement.xml
Lines 1527 to 1547 in c7324df
<Member MemberName="ValueKind"> | |
<MemberSignature Language="C#" Value="public System.Text.Json.JsonValueKind ValueKind { get; }" /> | |
<MemberSignature Language="ILAsm" Value=".property instance valuetype System.Text.Json.JsonValueKind ValueKind" /> | |
<MemberSignature Language="DocId" Value="P:System.Text.Json.JsonElement.ValueKind" /> | |
<MemberSignature Language="VB.NET" Value="Public ReadOnly Property ValueKind As JsonValueKind" /> | |
<MemberSignature Language="C++ CLI" Value="public:
 property System::Text::Json::JsonValueKind ValueKind { System::Text::Json::JsonValueKind get(); };" /> | |
<MemberSignature Language="F#" Value="member this.ValueKind : System.Text.Json.JsonValueKind" Usage="System.Text.Json.JsonElement.ValueKind" /> | |
<MemberType>Property</MemberType> | |
<AssemblyInfo> | |
<AssemblyName>System.Text.Json</AssemblyName> | |
<AssemblyVersion>4.0.0.0</AssemblyVersion> | |
</AssemblyInfo> | |
<ReturnValue> | |
<ReturnType>System.Text.Json.JsonValueKind</ReturnType> | |
</ReturnValue> | |
<Docs> | |
<summary>To be added.</summary> | |
<value>To be added.</value> | |
<remarks>To be added.</remarks> | |
</Docs> | |
</Member> |
<exception cref="T:System.InvalidOperationException">This value's <see cref="P:System.Text.Json.JsonElement.Type" /> is not <see cref="F:System.Text.Json.JsonValueKind.Array" />.</exception> | ||
<exception cref="T:System.IndexOutOfRangeException"> | ||
<paramref name="index" /> is not in the range [0, <see cref="M:System.Text.Json.JsonElement.GetArrayLength" />()).</exception> | ||
<exception cref="T:System.InvalidOperationException">This value's <see cref="P:System.Text.Json.JsonElement.ValueKind" /> is not <see cref="F:System.Text.Json.JsonValueKind.Array" />.</exception> |
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.
We are missing parameter description of this indexer:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonelement.item?view=netcore-3.0#System_Text_Json_JsonElement_Item_System_Int32_
dotnet-api-docs/xml/System.Text.Json/JsonElement.xml
Lines 787 to 795 in c7324df
<Docs> | |
<param name="index">To be added.</param> | |
<summary>Gets the value at a specified index when the current value is an <see cref="F:System.Text.Json.JsonValueKind.Array" />.</summary> | |
<value>To be added.</value> | |
<remarks>To be added.</remarks> | |
<exception cref="T:System.InvalidOperationException">This value's <see cref="P:System.Text.Json.JsonElement.ValueKind" /> is not <see cref="F:System.Text.Json.JsonValueKind.Array" />.</exception> | |
<exception cref="T:System.IndexOutOfRangeException"><paramref name="index" /> is not in the range [0, <see cref="M:System.Text.Json.JsonElement.GetArrayLength" />()).</exception> | |
<exception cref="T:System.ObjectDisposedException">The parent <see cref="T:System.Text.Json.JsonDocument" /> has been disposed.</exception> | |
</Docs> |
It doesn't exist in source either.
@bartonjs, can you please wordsmith something here to describe the JsonValueKind.Array
indexer?
https://github.com/dotnet/corefx/blob/9e994a5923d426f0e64ed3b8f9b93df6f214a42a/src/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs#L40-L61
I synced my dotnet-api-docs clone to the latest bits to ensure I had the latest xml API structure from the most recent reference assembly update. I also synced my corefx repo to the latest bits and built them to ensure I had the latest comments.
Area owners @ahsonkhan @steveharter please help double check that the descriptions are all accurate.
@mairaw @rpetrusha for language review.