Skip to content

Conversation

@mairaw
Copy link
Contributor

@mairaw mairaw commented Feb 18, 2019

No description provided.

@mairaw mairaw added the 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 label Feb 18, 2019
@mairaw mairaw added this to the February 2019 milestone Feb 18, 2019
@mairaw mairaw self-assigned this Feb 18, 2019
@mairaw mairaw requested a review from ahsonkhan February 18, 2019 03:11
@mairaw mairaw added the WIP label Feb 18, 2019
@mairaw mairaw changed the title add content for System.Text.Json APIs WIP add content for System.Text.Json APIs Feb 18, 2019
@mairaw mairaw force-pushed the system.text.json branch 2 times, most recently from 5075c29 to 523d5dd Compare February 20, 2019 07:59
@mairaw
Copy link
Contributor Author

mairaw commented Feb 22, 2019

This is still WIP. I should have made that clear earlier but wanted to make sure you saw I was loading the comments you wanted.
Appreciate the review comments so far.

@ahsonkhan
Copy link
Contributor

This is still WIP. I should have made that clear earlier but wanted to make sure you saw I was loading the comments you wanted.

Sounds good, thanks for letting me know :)

]]></format>
</remarks>
<exception cref="T:System.InvalidOperationException">
The JSON token value isn't a <see cref="F:System.Text.Json.JsonTokenType.Number" />.</exception>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should all of these InvalidOperationException messages use the more-Englishy The JSON token value isn't <see cref="Number"/> or the more programmery <see cref="TokenType" /> is not <see cref="Number" />?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It renders like this:
image

i think it's fine as-is?

@mairaw mairaw modified the milestones: February 2019, March 2019 Mar 7, 2019
@mairaw
Copy link
Contributor Author

mairaw commented Apr 5, 2019

@ahsonkhan @bartonjs thanks so much for your feedback! I believe I've addressed everything and I had some last questions before we can merge this.

@mairaw mairaw modified the milestones: March 2019, April 2019 Apr 5, 2019
@mairaw mairaw changed the title WIP add content for System.Text.Json APIs add content for System.Text.Json APIs Apr 9, 2019
@mairaw mairaw removed the WIP label Apr 9, 2019
<Interfaces />
<Docs>
<summary>To be added.</summary>
<summary>Provides the ability for the user to define custom behavior when reading JSON.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intention that this would be merged for preview3? I am asking because in the upcoming preview4 build, we moved MaxDepth to JsonReaderOptions (from JsonReaderState here: https://github.com/dotnet/dotnet-api-docs/pull/1880/files#diff-e0eccf9b88a4e527eed98ff46094dbdbR39).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's my intention 😄

<value>To be added.</value>
<remarks>To be added.</remarks>
<summary>Gets or sets a value that indicates whether the <see cref="T:System.Text.Json.Utf8JsonWriter" /> should skip structural validation and allow the user to write invalid JSON.</summary>
<value><see langword="true" /> to skip structural validation and allow invalid JSON; otherwise, <see langword="false" />: any attempts to write invalid JSON will result in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using : correct here? Can you share the link to the docs so I can see how it would look?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a large number of comments, @mairaw, but you can merge once you're satisfied with the content.

@mairaw
Copy link
Contributor Author

mairaw commented Apr 11, 2019

Thank you all for your feedback. I think I've handled all the comments I got. If everything looks good, we can merge tomorrow.

@mairaw mairaw merged commit 31b8fe9 into dotnet:master Apr 11, 2019
@mairaw mairaw deleted the system.text.json branch April 11, 2019 23:56
Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some leftover feedback since I hadn't fully gone through the Utf8JsonWriter.xml earlier.

<summary>To be added.</summary>
<remarks>To be added.</remarks>
<param name="bufferWriter">An instance of <see cref="T:System.Buffers.IBufferWriter`1" /> used as a destination for writing JSON text.</param>
<param name="state">A struct that contains the reader state. On the first call to the constructor, it should reflect a default state; otherwise, it should capture the state from the previous instance instance of the <see cref="T:System.Text.Json.Utf8JsonWriter" /> and pass that back.</param>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writer state, not reader state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the first call to the constructor, it should reflect a default state

I don't think that captures the intent. The state is being passed in to the ctor. Why is it reflecting anything?

it should capture the state from the previous instance instance of the

Not "it", the caller should capture the state...

<remarks>To be added.</remarks>
<param name="bufferWriter">An instance of <see cref="T:System.Buffers.IBufferWriter`1" /> used as a destination for writing JSON text.</param>
<param name="state">A struct that contains the reader state. On the first call to the constructor, it should reflect a default state; otherwise, it should capture the state from the previous instance instance of the <see cref="T:System.Text.Json.Utf8JsonWriter" /> and pass that back.</param>
<summary>Initializes a new instance of the <see cref="T:System.Text.Json.Utf8JsonWriter" /> class with the specified <paramref name="bufferWriter" />.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a class.

## Remarks
Unlike the <xref:System.Text.Json.Utf8JsonWriter>, which is a ref struct, the state can survive across async/await boundaries, and hence this type is required to provide support for reading
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<param name="escape">To be added.</param>
<summary>To be added.</summary>
<param name="utf8Value">The UTF-8 encoded value to be written as a JSON comment.</param>
<param name="escape"><see langword="false" /> to indicate the writer should assume the property name is properly escaped and skips the escaping step; otherwise, <see langword="true" />. This is an optional parameter and its default value is <see langword="true" />.</param>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API doesn't contain "property name". Looks like copy/paste error.

writer should assume the property name value is properly escaped

Suggested change
<param name="escape"><see langword="false" /> to indicate the writer should assume the property name is properly escaped and skips the escaping step; otherwise, <see langword="true" />. This is an optional parameter and its default value is <see langword="true" />.</param>
<param name="escape"><see langword="false" /> to indicate the writer should assume the value is properly escaped and skips the escaping step; otherwise, <see langword="true" />. This is an optional parameter and its default value is <see langword="true" />.</param>

<param name="escape">To be added.</param>
<summary>To be added.</summary>
<param name="value">The UTF-16 encoded value to be written as a UTF-8 transcoded JSON comment.</param>
<param name="escape"><see langword="false" /> to indicate the writer should assume the property name is properly escaped and skips the escaping step; otherwise, <see langword="true" />. This is an optional parameter and its default value is <see langword="true" />.</param>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here/below.

Suggested change
<param name="escape"><see langword="false" /> to indicate the writer should assume the property name is properly escaped and skips the escaping step; otherwise, <see langword="true" />. This is an optional parameter and its default value is <see langword="true" />.</param>
<param name="escape"><see langword="false" /> to indicate the writer should assume the value is properly escaped and skips the escaping step; otherwise, <see langword="true" />. This is an optional parameter and its default value is <see langword="true" />.</param>

<param name="escape">To be added.</param>
<summary>To be added.</summary>
<param name="value">The UTF-16 encoded value to be written as a UTF-8 transcoded JSON string element of a JSON array.</param>
<param name="escape"><see langword="false" /> to indicate the writer should assume the property name is properly escaped and skips the escaping step; otherwise, <see langword="true" />. This is an optional parameter and its default value is <see langword="true" />.</param>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too. Everywhere we have WriteStringValue overloads, the API doesn't contain a property name parameter.

Suggested change
<param name="escape"><see langword="false" /> to indicate the writer should assume the property name is properly escaped and skips the escaping step; otherwise, <see langword="true" />. This is an optional parameter and its default value is <see langword="true" />.</param>
<param name="escape"><see langword="false" /> to indicate the writer should assume the value is properly escaped and skips the escaping step; otherwise, <see langword="true" />. This is an optional parameter and its default value is <see langword="true" />.</param>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants