Add string-based WriteAsProperty overload to JsonElement.#37789
Add string-based WriteAsProperty overload to JsonElement.#37789ahsonkhan merged 2 commits intodotnet:masterfrom
Conversation
| /// <summary> | ||
| /// Write the element into the provided writer as a named object property. | ||
| /// </summary> | ||
| /// <param name="propertyName">The name for this value within the JSON object, as UTF-16 text.</param> |
There was a problem hiding this comment.
Since it's a string, the encoding shouldn't be mentioned. So ditch everything after the comma.
There was a problem hiding this comment.
(Or, thanks to the way the diff lined up, copy the one for ROS<char>, which similarly doesn't need to mention the encoding, since it's not bytes)
There was a problem hiding this comment.
I was following the format in the writer APIs, where ROS<byte> states UTF-8 encoding, ROS<char> states UTF-16 encoding, and the string overload doesn't specify at all.
For the string case, I don't specify the encodings, but kept it for the ROS<char> one.
There was a problem hiding this comment.
"UTF-16" really only applies to bytes. With char it's relevant where surrogates are concerned, but in that case char means UTF-16... so mentioning the encoding is unnecessary to the point that it's probably confusing.
I strongly advocate not having it for the ROS<char>, which is why it does not currently have it.
There was a problem hiding this comment.
OK, I will fix it here for consistency. I filed an issue for the writer APIs, though I am not fully convinced that it is necessary. Presumably, this applies to summary and param docs as well, correct (for both ROS<char> and string overloads)?
| /// The parent <see cref="JsonDocument"/> has been disposed. | ||
| /// </exception> | ||
| public void WriteAsProperty(string propertyName, Utf8JsonWriter writer) | ||
| => WriteAsProperty(propertyName.AsSpan(), writer); |
There was a problem hiding this comment.
Is the intent to let null succeed?
There was a problem hiding this comment.
Yes. null == "" (consistent with how the writer works today).
We need to agree on the semantics as part of https://github.com/dotnet/corefx/issues/34632, but I wanted to keep it as is until then.
|
|
||
| string propertyName = (string)entry.Key; | ||
| element.WriteAsProperty(propertyName.AsSpan(), writer); | ||
| element.WriteAsProperty(propertyName, writer); |
…efx#37789) * Add string-based WriteAsProperty overload to JsonElement. * Address PR feedback - remove UTF-16 from the comments. Commit migrated from dotnet/corefx@1897667
2 participants
This API is useful internally within JsonSerializer as well. See #37690 (comment)