Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Json string and boolean#40107

Merged
kasiabulat merged 18 commits intodotnet:masterfrom
kasiabulat:kasiabulat/json-string-and-boolean
Aug 10, 2019
Merged

Json string and boolean#40107
kasiabulat merged 18 commits intodotnet:masterfrom
kasiabulat:kasiabulat/json-string-and-boolean

Conversation

@kasiabulat
Copy link
Copy Markdown
Contributor

I added JsonString and JsonBoolean implementation.

addresses: #39922
cc: @joperezr @bartonjs @ericstj @ahsonkhan @terrajobst @JamesNK

Copy link
Copy Markdown
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

The description here says "addresses: #39922", but this only adds a few of the types, and the PR doesn't actually allow for these to be used in any meaningful way. Should we wait to merge this until it's actually possible to construct a JsonDocument from these? Otherwise we're sort of adding dead code / public surface area.

Comment thread src/System.Text.Json/src/System.Text.Json.csproj
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonBoolean.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonBoolean.cs
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonString.cs
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonString.cs Outdated
@ericstj
Copy link
Copy Markdown
Member

ericstj commented Aug 7, 2019

Should we wait to merge this until it's actually possible to construct a JsonDocument from these? Otherwise we're sort of adding dead code / public surface area.

This represents individually usable / testable chunks of the overall feature. We're OK with checking them in those chunks as we've committed to finishing the whole feature. If for some reason we cannot finish we'll pull out these chunks, but I don't forsee that happening.

@stephentoub
Copy link
Copy Markdown
Member

stephentoub commented Aug 7, 2019

usable / testable chunks

testable, yes. usable, no. But ok; it's fine if we're committed to either finishing it or backing this out.

@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Aug 7, 2019

Something to figure out is what happens when you call ToString on these types. Do you return the JSON representation or the .NET representation?

JsonBoolean b = new JsonBoolean(true);
b.ToString(); // "True" or "true"

That question becomes important around JsonString. Should the result from ToString be escaped and surrounded by double quotes, or should it be the .NET string? Originally Json.NET returned the quoted/escaped JSON representation and people didn't like it. Now ToString on all the JToken types return the JSON representation except JValue with a string, which just returns the internal string.

Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonBoolean.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonBoolean.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonString.cs
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonString.cs Outdated
Comment thread src/System.Text.Json/tests/JsonBooleanTests.cs
Comment thread src/System.Text.Json/tests/JsonBooleanTests.cs
Comment thread src/System.Text.Json/tests/JsonBooleanTests.cs
Comment thread src/System.Text.Json/tests/JsonStringTests.cs Outdated
Comment thread src/System.Text.Json/tests/JsonStringTests.cs Outdated
Comment thread src/System.Text.Json/tests/JsonStringTests.cs Outdated
Comment thread src/System.Text.Json/ref/System.Text.Json.cs
Comment thread src/System.Text.Json/ref/System.Text.Json.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonBoolean.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonBoolean.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonString.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonString.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonString.cs
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonString.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonBoolean.cs
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonString.cs
Comment thread src/System.Text.Json/src/System/Text/Json/Node/JsonString.cs Outdated
@benaadams
Copy link
Copy Markdown
Member

benaadams commented Aug 8, 2019

@benaadams - you added that attribute to SortVersion here, along with a bunch of other places (dotnet/coreclr#21765), was it necessary?

It was so null checks (== null) would return to fast null checks rather than making a slower call to the overload

@kasiabulat
Copy link
Copy Markdown
Contributor Author

kasiabulat commented Aug 9, 2019

Why is ReadonlySpan behaving so differently on those platforms? What would be the best way to solve it? (I can just exclude the test for Windows NETFX_x86_Release))

Comment thread src/System.Text.Json/tests/JsonStringTests.cs Outdated
@kasiabulat kasiabulat merged commit a7c0f96 into dotnet:master Aug 10, 2019
@kasiabulat kasiabulat deleted the kasiabulat/json-string-and-boolean branch August 16, 2019 22:26
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* API to implement added
* JsonBoolean implementation added
* JsonString implementation added
* JsonBoolean tests added
* JsonString tests added
* review comments included
* 100% lines and branches covered
* framework fixes

Commit migrated from dotnet/corefx@a7c0f96
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants