From f2a47ddcf88ddf66c11b6270d0815d6c73e486ef Mon Sep 17 00:00:00 2001 From: Immo Landwerth Date: Tue, 12 Feb 2019 12:04:19 -0800 Subject: [PATCH 1/3] Add notes --- 2019/System.Text.Json-Serialization/README.md | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 2019/System.Text.Json-Serialization/README.md diff --git a/2019/System.Text.Json-Serialization/README.md b/2019/System.Text.Json-Serialization/README.md new file mode 100644 index 0000000..2679db6 --- /dev/null +++ b/2019/System.Text.Json-Serialization/README.md @@ -0,0 +1,31 @@ +# System.Text.Json (Serialization) + +Status: **Review not complete** | +[API Ref](https://github.com/dotnet/corefx/issues/34372) | +[Video](https://www.youtube.com/watch?v=I_CNwnkNDNA) + +## Notes + +* We're shooting for an MVP in the .NET Core 3.0 timeframe, mostly due to + time constraints but also because baking a serializer takes time +* We should compare & contrast usability +* "What's the percentage of customers we want to be happy with the built-in JSON + serialization." +* We shouldn't return `Span`; we should let the caller pass in the buffer to + fill. This likely also requires `out` parameters to indicate how many bytes + were written. +* The APIs returning `ValueTask` should probably return `Task` +* We should avoid the `To` and `From` prefixes, we should go with something simpler + - e.g. `JsonSerializer.Read`, `JsonSerializer.ReadAsync`, `JsonSerializer.Write`, `JsonSerializer.WriteAsync` +* Should we have `Utf8` in the name somewhere? How would we grow the serializer + to support other encodings, such as UTF16? +* Should the methods like `ToJson` and `ToJsonString` that only take `object` + also take a `Type` or be generic? +* The nullable properties on the options class probably shouldn't be nullable + because there is only one global behavior. + - We believe we should strive for the least confusion values, which would be + these: + - `SerializeNullValues`: `true` + - `DeserializeNullValues`: `true` + - `CaseInsensitivePropertyName`: `false` +* We should consider moving `MaxDepth` into reader/writer options \ No newline at end of file From bfd3ccf8e532e1c9d56251038b21202ececccc54 Mon Sep 17 00:00:00 2001 From: Immo Landwerth Date: Thu, 14 Feb 2019 11:51:18 -0800 Subject: [PATCH 2/3] Address Ahson's feedback --- 2019/System.Text.Json-Serialization/README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/2019/System.Text.Json-Serialization/README.md b/2019/System.Text.Json-Serialization/README.md index 2679db6..d004bde 100644 --- a/2019/System.Text.Json-Serialization/README.md +++ b/2019/System.Text.Json-Serialization/README.md @@ -14,7 +14,19 @@ Status: **Review not complete** | * We shouldn't return `Span`; we should let the caller pass in the buffer to fill. This likely also requires `out` parameters to indicate how many bytes were written. + - Make them regular static methods (rather than extension methods). * The APIs returning `ValueTask` should probably return `Task` +* The writer APIs should take the value and then the writer (source -> destination) + - IOW, this: + ```C# + public static ValueTask ToJsonAsync(this PipeWriter writer, object value, JsonConverterOptions options = null, CancellationToken cancellationToken = default); + public static ValueTask ToJsonsync(this Stream stream, object value, JsonConverterOptions options = null, CancellationToken cancellationToken = default); + ``` + - Should be this: + ```C# + public static ValueTask ToJsonAsync(object value, PipeWriter writer, JsonConverterOptions options = null, CancellationToken cancellationToken = default); + public static ValueTask ToJsonsync(object value, Stream stream, JsonConverterOptions options = null, CancellationToken cancellationToken = default); + ``` * We should avoid the `To` and `From` prefixes, we should go with something simpler - e.g. `JsonSerializer.Read`, `JsonSerializer.ReadAsync`, `JsonSerializer.Write`, `JsonSerializer.WriteAsync` * Should we have `Utf8` in the name somewhere? How would we grow the serializer From 4f2f720ff8e23bc6bacb2144a249fa2e8556066d Mon Sep 17 00:00:00 2001 From: Immo Landwerth Date: Thu, 14 Feb 2019 11:52:03 -0800 Subject: [PATCH 3/3] Address Jan's feedback --- 2019/System.Text.Json-Serialization/README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/2019/System.Text.Json-Serialization/README.md b/2019/System.Text.Json-Serialization/README.md index d004bde..fa88ddc 100644 --- a/2019/System.Text.Json-Serialization/README.md +++ b/2019/System.Text.Json-Serialization/README.md @@ -27,6 +27,9 @@ Status: **Review not complete** | public static ValueTask ToJsonAsync(object value, PipeWriter writer, JsonConverterOptions options = null, CancellationToken cancellationToken = default); public static ValueTask ToJsonsync(object value, Stream stream, JsonConverterOptions options = null, CancellationToken cancellationToken = default); ``` +* The APIs taking `PipeWriter` might be problematic because it implies that the + JSON component depends on pipelines. We probably want this the other way + around. * We should avoid the `To` and `From` prefixes, we should go with something simpler - e.g. `JsonSerializer.Read`, `JsonSerializer.ReadAsync`, `JsonSerializer.Write`, `JsonSerializer.WriteAsync` * Should we have `Utf8` in the name somewhere? How would we grow the serializer