Add WritePropertyName standlone API on the Utf8JsonWriter. by ahsonkhan · Pull Request #38864 · dotnet/corefx

ahsonkhan · 2019-06-25T06:14:57Z

Fixes https://github.com/dotnet/corefx/issues/36639#issuecomment-500748970

cc @steveharter, @bartonjs, @JeremyKuhne, @rynowak

JsonTokenType.PropertyName

src/System.Text.Json/src/Resources/Strings.resx

src/System.Text.Json/src/System/Text/Json/ThrowHelper.cs

stephentoub · 2019-06-25T13:54:43Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.String.cs

+        /// The property name should already be escaped when the instance of <see cref="JsonEncodedText"/> was created.
+        /// </remarks>
+        /// <exception cref="InvalidOperationException">
+        /// Thrown if this would result in an invalid JSON to be written (while validation is enabled).


"would result in an invalid JSON to be written" is strange grammatically. How about "would result in invalid JSON being written"?

stephentoub · 2019-06-25T13:56:51Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.String.cs

+        /// <param name="propertyName">The JSON encoded property name of the JSON object to be transcoded and written as UTF-8.</param>
+        /// </summary>
+        /// <remarks>
+        /// The property name should already be escaped when the instance of <see cref="JsonEncodedText"/> was created.


"The property name should already be escaped when the instance of JsonEncodedText was created." is strange . What is it trying to convey? Is there something the user needed to do but didn't? Is this stating a fact about something that must be the case?

I agree this remark is weird. It's like taking an int and remarking the value has to be between int.MinValue and int.MaxValue inclusive... it's a truth provided by the type system.

stephentoub · 2019-06-25T13:58:01Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.String.cs

+                }
+                else
+                {
+                    // Cannot create a span directly since it gets assigned to parameter and passed down.


Are we not able to work around this now with readonly members?

Copy-pasted from existing code, so will have to fix this across the board. Out-of-scope for this PR.

stephentoub · 2019-06-25T13:59:57Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.String.cs

+
+            Span<byte> output = _memory.Span;
+
+            if (_currentDepth < 0)


I had to go find the comment on _currentDepth to understand how a depth could be negative. Consider instead creating a private property that does this check and is appropriately named to describe what it's actually checking.

Copy-pasted from existing code, so will have to fix this across the board. Out-of-scope for this PR.

src/System.Text.Json/src/Resources/Strings.resx

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.SignedNumber.cs

rynowak · 2019-06-25T14:52:50Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.String.cs

+        /// <summary>
+        /// Writes the property name (as a JSON string) as the first part of a name/value pair of a JSON object.
+        /// </summary>
+        /// <param name="propertyName">The property name of the JSON object to be transcoded and written as UTF-8.</param>


transcoded and written as UTF-8

Does it really make sense to mention this detail everywhere?

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.String.cs

bartonjs · 2019-06-25T15:46:20Z

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.String.cs

    {
+        /// <summary>
+        /// Writes the pre-encoded property name (as a JSON string) as the first part of a name/value pair of a JSON object.
+        /// <param name="propertyName">The JSON encoded property name of the JSON object to be transcoded and written as UTF-8.</param>


Who said anything about transcoding? That's an irrelevant implementation detail (if the JsonEncodedText doesn't pre-transcode, then the only way to avoid transcoding is to use the ROS-byte overload... which runs the escaper. So it's not worth mentioning, especially in the param tag... remarks if you must.)

Suggested change

/// <param name="propertyName">The JSON encoded property name of the JSON object to be transcoded and written as UTF-8.</param>

/// <param name="propertyName">The name of the property to write.</param>

src/System.Text.Json/tests/Utf8JsonWriterTests.cs

ahsonkhan · 2019-06-25T19:49:06Z

A lot of the feedback here was around comments/documentation re-wording which was primarily copy-pasted from existing APIs. I am going to defer addressing that feedback to scope this PR down to what is relevant just to this change and do a larger comments re-wording in the future, especially given the time constraint (and to ublock other work around converters - cc @steveharter).

Appreciate the input.

https://github.com/dotnet/corefx/issues/38896

ghost · 2019-06-25T20:28:06Z

Hello @ahsonkhan!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

bartonjs

Waiting on tests to show that WriteBoolean(propertyName, value) and friends are not valid (when validation is not suppressed) after WritePropertyName

ahsonkhan · 2019-06-25T22:01:48Z

Waiting on tests to show that WriteBoolean(propertyName, value) and friends are not valid (when validation is not suppressed) after WritePropertyName

Up-stack consumers are waiting on this change and this PR is blocking some of the converter work. I would like to get this in and will address adding more tests for those cases separately. I do have very similar tests for other "WriteX(propertyName, value)", but not for all TokenTypes, so I am fairly confident this works as expected.

I would like to avoid resetting CI at this point.

…refx#38864) * Add WritePropertyName standlone API on the Utf8JsonWriter. * Fix write indented, add more tests, and more debug.asserts. * Remove _isProperty field and rely on _tokenType == JsonTokenType.PropertyName * Auto-gen the ref. * Address PR feedback. Commit migrated from dotnet/corefx@a5b35f5

ahsonkhan added 4 commits June 24, 2019 13:39

Add WritePropertyName standlone API on the Utf8JsonWriter.

38308e8

Fix write indented, add more tests, and more debug.asserts.

785e7e4

Remove _isProperty field and rely on _tokenType ==

5fd7285

JsonTokenType.PropertyName

Auto-gen the ref.

50be2f3

ahsonkhan added the area-System.Text.Json label Jun 25, 2019

ahsonkhan added this to the 3.0 milestone Jun 25, 2019

ahsonkhan self-assigned this Jun 25, 2019

stephentoub reviewed Jun 25, 2019

View reviewed changes

src/System.Text.Json/src/Resources/Strings.resx Outdated Show resolved Hide resolved

stephentoub reviewed Jun 25, 2019

View reviewed changes

src/System.Text.Json/src/System/Text/Json/ThrowHelper.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jun 25, 2019

View reviewed changes

src/System.Text.Json/src/System/Text/Json/ThrowHelper.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jun 25, 2019

View reviewed changes

rynowak reviewed Jun 25, 2019

View reviewed changes

src/System.Text.Json/src/Resources/Strings.resx Outdated Show resolved Hide resolved

rynowak reviewed Jun 25, 2019

View reviewed changes

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.SignedNumber.cs Show resolved Hide resolved

rynowak reviewed Jun 25, 2019

View reviewed changes

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.String.cs Show resolved Hide resolved

bartonjs reviewed Jun 25, 2019

View reviewed changes

src/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteProperties.String.cs Show resolved Hide resolved

bartonjs reviewed Jun 25, 2019

View reviewed changes

src/System.Text.Json/tests/Utf8JsonWriterTests.cs Show resolved Hide resolved

Address PR feedback.

10f53cb

ahsonkhan added the auto-merge Automatically merge PR once CI passes. label Jun 25, 2019

bartonjs removed the auto-merge Automatically merge PR once CI passes. label Jun 25, 2019

bartonjs suggested changes Jun 25, 2019

View reviewed changes

ahsonkhan added the auto-merge Automatically merge PR once CI passes. label Jun 25, 2019

ghost merged commit a5b35f5 into dotnet:master Jun 25, 2019

ahsonkhan deleted the WritePropertyName branch June 26, 2019 00:38

ahsonkhan mentioned this pull request Jun 26, 2019

Add more Utf8JsonWriter.WritePropertyName test cases. #38921

Merged

steveharter mentioned this pull request Jun 26, 2019

Remove converter Write() that takes property name #38932

Merged

ahsonkhan mentioned this pull request Jul 16, 2019

Fix Utf8JsonWriter xml docs #39092

Merged

ahsonkhan mentioned this pull request Feb 1, 2020

The Utf8JsonWriter API xml documentation could be improved dotnet/runtime#30031

Closed

This pull request was closed.

	/// <param name="propertyName">The JSON encoded property name of the JSON object to be transcoded and written as UTF-8.</param>
	/// <param name="propertyName">The name of the property to write.</param>

Conversation

ahsonkhan commented Jun 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephentoub Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

stephentoub Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartonjs Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

stephentoub Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

stephentoub Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rynowak Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bartonjs Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahsonkhan commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 25, 2019

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

bartonjs left a comment

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stephentoub Jun 25, 2019 •

edited

Loading

bartonjs Jun 25, 2019 •

edited

Loading

ahsonkhan commented Jun 25, 2019 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ahsonkhan commented Jun 25, 2019 •

edited

Loading