Implement GetComment API by WinCPP · Pull Request #35705 · dotnet/corefx

WinCPP · 2019-03-01T22:04:09Z

Implement GetComment API with test cases.

Fixes #33347.

Please note that this depends on PR #33393 for successful handling of single line comment with different line terminators.

@ahsonkhan @terrajobst please review.

WinCPP · 2019-03-02T09:36:20Z

@dotnet-bot test corefx-ci (Windows NETFX_x86_Release)

WinCPP · 2019-03-02T09:38:13Z

@dotnet-bot test corefx-ci (Windows NETFX_x86_Release) please

MarcoRossignoli · 2019-03-02T09:58:53Z

@WinCPP we're now on Azure pipelines check here for new commands https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/pullrequest-builds.md

WinCPP · 2019-03-02T10:53:07Z

@MarcoRossignoli thank you for the guidance. For last several months, I was under the ice, à la Captain America, when this change happened; just resumed contributing.

And I forgot that this PR depends on another one #33393 for successful builds, which I had commented as well in the description 🤦‍♂️, so I have to wait. But, good that I hit this issue and I learnt the new process.

Thanks again!

ahsonkhan · 2019-03-01T22:10:11Z

        }

+        /// <summary>
+        /// Reads value of the next JSON token for a comment from the source transcoded as a <see cref="string"/>,


Comment could be clearer: Maybe something like:
Reads the next JSON token value from the source as a comment, transcoded as a <see cref="string"/>, preserving the comment delimiters.

Based on #35705 (comment), let's not preserve the comment delimiters.

Would it make sense to strip the comment delimiters (\\, /* and */) and the line separators (\r, \r\n and \n) during parsing where the ValueSpan or ValueSequence for the JsonTokenType.Comment is prepared? In that case, should I do the changes in PR #33393 or should I do them in this PR? If it is to be done in this PR, then it would help if we have an early closure on #33393 so that this code can be rebased to include work for \r\n.

Edit 1
Hmm... I see that ConsumeString already excludes the quotes, so likewise it would be more logical to implement the functionality mentioned above...

Edit 2
Query: IIUC this line in JsonReaderHelper.CountNewLines needs to handle \r on its own as a line separator, and also ensure that \r\n are handled properly... ?

Would it make sense to strip the comment delimiters (\\, /* and */) and the line separators (\r, \r\n and \n) during parsing where the ValueSpan or ValueSequence for the JsonTokenType.Comment is prepared?

Yes, it would.

In that case, should I do the changes in PR #33393 or should I do them in this PR?

Let's stage it in smaller, clearer chunks, where the change is isolated. So, in this order:

Utf8JsonReader.cs - Add support for single line comments ending on \r, \r\n #33393 - Add support for single line comments ending on \r, \r\n

Open up a new PR - Change ValueSpan/ValueSequence to exclude the comment delimiters, effectively undoing parts of 4a2aa63 from Add JsonUtf8Reader (for ReadOnlySpan) along with unit tests #33216

This PR (Implement GetComment API #35705) to expose new GetComment API

What do you think of this approach?

Query: IIUC this line in JsonReaderHelper.CountNewLines needs to handle \r on its own as a line separator, and also ensure that \r\n are handled properly... ?

Unless this is blocking some of the work above, I would file a new issue for this and investigate/fix separately. Some questions we likely want to resolve first: What does Json.NET return for line number when comments end with just \r (for Windows vs Unix)? Do we double count line number \r\n or treat it as a single line increment?

cc @JamesNK

Let's stage it in smaller, clearer chunks, where the change is isolated. So, in this order:

Utf8JsonReader.cs - Add support for single line comments ending on \r, \r\n #33393 - Add support for single line comments ending on \r, \r\n

Open up a new PR - Change ValueSpan/ValueSequence to exclude the comment delimiters, effectively undoing parts of 4a2aa63 from Add JsonUtf8Reader (for ReadOnlySpan) along with unit tests #33216

This PR (Implement GetComment API #35705) to expose new GetComment API

What do you think of this approach?

Sounds like a plan! Thanks! So, I am working on #33393 on priority to finish step 1, and then pull in the code changes, once submitted into master, into step 2. So will put this (and new) PR a bit on hold. Kindly see updates for #33393.

Do we double count line number \r\n or treat it as a single line increment?

Json.NET treats it as a single line increment

JamesNK · 2019-03-06T03:08:59Z

I think this PR is lite on tests. This is a Json.NET unit test for single line comments:

https://github.com/JamesNK/Newtonsoft.Json/blob/7217c484e9705b5e76585c8b7fcd489c8e021c23/Src/Newtonsoft.Json.Tests/JsonTextReaderTests/MiscTests.cs#L784-L919

It tests reading them in every place they could appear in JSON.

WinCPP · 2019-03-06T03:44:32Z

@JamesNK, this PR is for new API to fetch comments and not really related to carriage return usage across JSON. However I pointed it out because I saw we are not incrementing the line count.

I agree we need more tests, but could that be separate exercise? Or rather part of #33393 which is specifically for single line comments. Please refer to tests over there.

JamesNK · 2019-03-07T02:29:29Z

I'm concerned in general about how robust the reader's support of comments are. @ahsonkhan would the reader successfully parse https://github.com/JamesNK/Newtonsoft.Json/blob/7217c484e9705b5e76585c8b7fcd489c8e021c23/Src/Newtonsoft.Json.Tests/JsonTextReaderTests/MiscTests.cs#L784-L919 ?

ahsonkhan · 2019-03-07T02:40:26Z

would the reader successfully parse

We should try and see (and add a test like that to our test bed). It might uncover some bugs around corner cases.

Let me take a look.

ahsonkhan · 2019-03-07T04:42:36Z

Excluding some the things that Json.NET supports, the reader seems to parse the comments fine.

karelz · 2019-03-18T16:51:19Z

What are next steps for this PR? Are more changes needed?

WinCPP · 2019-03-18T18:56:21Z

What are next steps for this PR? Are more changes needed?

@karelz I am reverting some of the code changes in the last commit in this PR as they will now be taken care by #35934. Also will take care of other review comments in here and the conflicts and submit another commit...

WinCPP · 2019-03-18T19:34:00Z

Also as per discussion with @ahsonkhan in this comment, #35934 (step 2 in the comment) needs to go in so that I can rebase this PR and do some changes in comments related tests.

WinCPP · 2019-03-18T21:26:39Z

@karelz @ahsonkhan
Current status:

Reverted changes to comment parsing logic as discussed above (here)
Resolved merge conflicts with master

Next steps:

Once Consume comments changes #35934 is approved and merged to master, I will rebase this PR. Until then few test cases in this PR will fail.
Also I think I should change the existing test cases, as part of this PR, to use new GetComment API in all those places where following type of code is used to access comment string? Appreciate inputs.

// current usage
string commentText = Encoding.UTF8.GetString(json.HasValueSequence ? json.ValueSequence.ToArray() : json.ValueSpan.ToArray());

// instead do the following
string commentText = json.GetComment();

karelz · 2019-04-01T20:39:02Z

Marking as blocked on #35934 then.

ahsonkhan · 2019-05-17T22:38:02Z

Marking as blocked on #35934 then.

#35934 has been merged and hence progress on this PR is unblocked.

WinCPP · 2019-05-19T09:25:55Z

@ahsonkhan I rebased this PR on master in which #35934 is now available. There were very few places in existing test cases where GetComment change was needed.

The build is however failing. Not sure what to do. Thanks!

ahsonkhan · 2019-05-19T22:24:19Z

The build is however failing.

Do you mean locally or the CI leg corefx-ci (Linux arm64_Release)?

https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F35705~2Fmerge/test~2Ffunctional~2Fcli~2Finnerloop~2F/20190518.22

That looks unrelated, likely something to do with the Ubuntu.1604.Arm64 machine/infra.

For example:
System.ComponentModel.EventBasedAsync.Tests

Ubuntu.1604.Arm64.Open-arm64:Release
Details from Job 9679d62e-d93b-41c4-ba1b-aecc24547a3b
ExitCode: 999 
Ran on Machine: DDARM64S-028 
Get Repro environment 
Console 
 Executed on DDARM64S-028 using docker image mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-16.04-helix-arm64v8-b049512-20190321153539
running $HELIX_CORRELATION_PAYLOAD/scripts/decd945f4caa4f9e9b6936f0f39cd7c6/execute.sh in /home/helixbot/dotnetbuild/work/9679d62e-d93b-41c4-ba1b-aecc24547a3b/Work/184ba070-012f-40e8-9006-523c440565bc/Exec max 900 seconds

ahsonkhan

Otherwise, LGTM.

ahsonkhan · 2019-05-19T22:38:16Z

+                // single line comments
+                foreach (var raw in rawComments)
+                {
+                    var str = string.Format(raw, "");


Why do we need to do this?

In here, each of the line in the rawComments works as format string. The {0} in each is replaced with "" and generates a single line comment since it is prefixed with // in the loop.

Whereas, in the loop at 3378 same is replaced with \n and the resultant string when enclosed in /* and */ makes a new line comment...

Of course, I could have let {0} remain as it is... but thought it would be cleaner...

ahsonkhan · 2019-05-19T22:41:12Z

+            var state = new JsonReaderState(options: new JsonReaderOptions { CommentHandling = JsonCommentHandling.Allow });
+            var reader = new Utf8JsonReader(dataUtf8, isFinalBlock: true, state);
+            bool commentFound = false;
+            while (reader.Read())


This test could be simplified since it is only a single token within the json.

Assert.True(reader.Read()); Assert.Equal(JsonTokenType.Comment, reader.TokenType); string comment = reader.GetComment(); Assert.Equal(expected, comment); Assert.NotEqual(Regex.Unescape(expected), comment);

* CoreFx dotnet/corefx#33347: Implement GetComment API * Generate References as per correct steps * Exclude delims & line separators, new unescape test * Revert consume comment changes, test case fixes * Partial revert of previous incorrect online merge * fixed another online merge conflict resolution slippage * Intermediate rebase from master * Reworked files * Reworked files 2 * Fix coding sytle as per review comments Commit migrated from dotnet/corefx@53aaaf3

ahsonkhan added the area-System.Text.Json label Mar 1, 2019

ahsonkhan added this to the 3.0 milestone Mar 1, 2019

WinCPP force-pushed the getcomment branch from 1691426 to 16024ef Compare March 2, 2019 04:24

ahsonkhan reviewed Mar 3, 2019

View reviewed changes

Comment thread src/System.Text.Json/tests/Utf8JsonReaderTests.TryGet.cs Outdated

karelz assigned WinCPP, ahsonkhan and steveharter Mar 4, 2019

WinCPP mentioned this pull request Mar 6, 2019

Utf8JsonReader.cs - Add support for single line comments ending on \r, \r\n #33393

Merged

WinCPP mentioned this pull request Mar 11, 2019

Consume comments changes #35934

Merged

karelz changed the title ~~CoreFx #33347: Implement GetComment API~~ Implement GetComment API Mar 18, 2019

karelz added the blocked Issue/PR is blocked on something - see comments label Apr 1, 2019

ahsonkhan removed the blocked Issue/PR is blocked on something - see comments label May 17, 2019

WinCPP force-pushed the getcomment branch from c0cdcd6 to c623572 Compare May 18, 2019 11:07

ahsonkhan approved these changes May 19, 2019

View reviewed changes

WinCPP added 10 commits May 20, 2019 21:00

CoreFx #33347: Implement GetComment API

1aadf3c

Generate References as per correct steps

98f7e2b

Exclude delims & line separators, new unescape test

78c73f1

Revert consume comment changes, test case fixes

2d578a8

Partial revert of previous incorrect online merge

f01613a

fixed another online merge conflict resolution slippage

08de98c

Intermediate rebase from master

39db76c

Reworked files

bdf2d03

Reworked files 2

196d683

Fix coding sytle as per review comments

5181c84

WinCPP force-pushed the getcomment branch from 359959b to 5181c84 Compare May 20, 2019 18:35

ahsonkhan approved these changes May 20, 2019

View reviewed changes

ahsonkhan merged commit 53aaaf3 into dotnet:master May 21, 2019

WinCPP deleted the getcomment branch September 9, 2019 03:40

Conversation

WinCPP commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WinCPP commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WinCPP commented Mar 2, 2019

Uh oh!

MarcoRossignoli commented Mar 2, 2019

Uh oh!

WinCPP commented Mar 2, 2019

Uh oh!

ahsonkhan Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Mar 3, 2019

Choose a reason for hiding this comment

Uh oh!

WinCPP Mar 3, 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!

ahsonkhan Mar 4, 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!

WinCPP Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

JamesNK Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JamesNK commented Mar 6, 2019

Uh oh!

WinCPP commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesNK commented Mar 7, 2019

Uh oh!

ahsonkhan commented Mar 7, 2019

Uh oh!

ahsonkhan commented Mar 7, 2019

Uh oh!

karelz commented Mar 18, 2019

Uh oh!

WinCPP commented Mar 18, 2019

Uh oh!

WinCPP commented Mar 18, 2019

Uh oh!

WinCPP commented Mar 18, 2019

Uh oh!

karelz commented Apr 1, 2019

Uh oh!

ahsonkhan commented May 17, 2019

Uh oh!

WinCPP commented May 19, 2019

Uh oh!

ahsonkhan commented May 19, 2019

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahsonkhan May 19, 2019

Choose a reason for hiding this comment

Uh oh!

WinCPP May 20, 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!

ahsonkhan May 19, 2019

Choose a reason for hiding this comment

Uh oh!

WinCPP commented Mar 1, 2019 •

edited

Loading

WinCPP commented Mar 2, 2019 •

edited

Loading

WinCPP Mar 3, 2019 •

edited

Loading

ahsonkhan Mar 4, 2019 •

edited

Loading

WinCPP commented Mar 6, 2019 •

edited

Loading

WinCPP May 20, 2019 •

edited

Loading