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

Utf8JsonWriter and JsonElement, redux #29012

Closed
bartonjs opened this issue Mar 19, 2019 · 1 comment
Closed

Utf8JsonWriter and JsonElement, redux #29012

bartonjs opened this issue Mar 19, 2019 · 1 comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@bartonjs
Copy link
Member

The original JsonDocument API proposal (#33968) had instance methods on Utf8JsonWriter which could (deeply) write a JsonElement.

-    public partial struct Utf8JsonWriter
-    {
-        public void WriteElement(string propertyName, JsonElement value, bool escape = true);
-        public void WriteElement(ReadOnlySpan<char> propertyName, JsonElement value, bool escape = true);
-        public void WriteElement(ReadOnlySpan<byte> utf8PropertyName, JsonElement value, bool escape = true);
-        public void WriteElementValue(JsonElement element, bool escape = true);
-    }

This was viewed, after the fact, as a layering violation.

The new proposal is

public partial struct JsonElement
{
    public void WriteValueTo(ref Utf8JsonWriter writer);
    public void WriteTo(ref Utf8JsonWriter writer, ReadOnlySpan<char> propertyName);
    public void WriteTo(ref Utf8JsonWriter writer, ReadOnlySpan<byte> utf8PropertyName);
}

Using the Utf8JsonWriter convention that Value is used when not a property, and no extra modifier is used for a property.

@terrajobst
Copy link
Member

terrajobst commented Mar 28, 2019

Video

Usability wise, this proposal seems less than ideal. Not sure I personal buy the layering argument, but if we feel strongly we should rename the method and swap the arguments:

public partial struct JsonElement
{
    public void WriteAsValue(ref Utf8JsonWriter writer);
    public void WriteAsProperty(ReadOnlySpan<char> propertyName, ref Utf8JsonWriter writer);
    public void WriteAsProperty(ReadOnlySpan<byte> utf8PropertyName, ref Utf8JsonWriter writer);
}

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 13, 2020
@bartonjs bartonjs removed their assignment Jul 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Projects
None yet
Development

No branches or pull requests

3 participants