Skip to content
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

Additional JsonNode functionality #87381

Merged
merged 33 commits into from
Jun 27, 2023
Merged

Conversation

RaymondHuy
Copy link
Contributor

Resolve #56592

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 11, 2023
@ghost
Copy link

ghost commented Jun 11, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Resolve #56592

Author: RaymondHuy
Assignees: -
Labels:

area-System.Text.Json, new-api-needs-documentation

Milestone: -

@RaymondHuy
Copy link
Contributor Author

Hi @eiriktsarpalis @layomia could you help me review this PR ?

case JsonValueKind.Number:
case JsonValueKind.True:
case JsonValueKind.False:
return jsonElementCurrent.GetRawValue().Span.SequenceEqual(jsonElementNodeOther._value.GetRawValue().Span);
Copy link
Member

Choose a reason for hiding this comment

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

This does not account for escaped strings when you have JsonValueKind.String

Copy link
Contributor Author

@RaymondHuy RaymondHuy Jun 26, 2023

Choose a reason for hiding this comment

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

Hi @eiriktsarpalis I can get this test passed:
`

    [Fact]
    public static void DeepEquals_EscapedString()
    {
        JsonValue jsonValue = JsonValue.Create(JsonDocument.Parse("\"It\'s alright\"").RootElement);
        JsonValue escapedJsonValue = JsonValue.Create(JsonDocument.Parse("\"It\u0027s alright\"").RootElement);
        Assert.True(JsonNode.DeepEquals(escapedJsonValue, jsonValue));
    }

`
or maybe I misunderstand your meaning, could you give me an example test case for it ?

Copy link
Member

Choose a reason for hiding this comment

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

That's what I meant. I'm actually surprised that the test is passing, so I would guess that GetRawValue() actually does unescaping. In any case, I think it might make sense to call ValueEquals specifically when you have string value kinds (since IIRC that kind is supported by that method.

Copy link
Member

Choose a reason for hiding this comment

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

At the same time you could call GetBoolean() for the case of True/False.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @eiriktsarpalis , I have updated it and added test case for escaped string case.

Copy link
Member

Choose a reason for hiding this comment

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

At the same time you could call GetBoolean() for the case of True/False.

Actually, wondering if just comparing with jsonElementNodeOther.ValueKind should suffice in this case:

                        case JsonValueKind.True:
                        case JsonValueKind.False:
                            return jsonElementCurrent.ValueKind == jsonElementNodeOther.ValueKind;

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

This looks great. If you could address the final test feedback I can merge this. 🎉

@eiriktsarpalis eiriktsarpalis merged commit dd079f5 into dotnet:main Jun 27, 2023
102 of 103 checks passed
@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Jun 27, 2023
@gregsdennis
Copy link
Contributor

Thanks @RaymondHuy and @eiriktsarpalis! This covers a lot of the functionality I have in Json.More.Net!

@ghost ghost locked as resolved and limited conversation to collaborators Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional JsonNode functionality
4 participants