Skip to content

Add JsonNumberHandling(Attribute) & support for (de)serializing numbers from/to string - #39363

Merged
layomia merged 4 commits into
dotnet:masterfrom
layomia:json_number
Jul 21, 2020
Merged

Add JsonNumberHandling(Attribute) & support for (de)serializing numbers from/to string#39363
layomia merged 4 commits into
dotnet:masterfrom
layomia:json_number

Conversation

@layomia

@layomialayomia commented Jul 15, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#30255.

@layomialayomia added this to the 5.0.0 milestone Jul 15, 2020
@layomialayomia self-assigned this Jul 15, 2020
@Dotnet-GitSync-Bot

Dotnet-GitSync-Bot commented Jul 15, 2020

Copy link
Copy Markdown
Collaborator

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, to 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.

jozkee
jozkee reviewed Jul 15, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs Outdated
@layomia
layomiaforce-pushed the json_number branch 3 times, most recently from bfb01c9 to 29d317fCompareJuly 15, 2020 17:20
jozkee
jozkee reviewed Jul 15, 2020
View reviewed changes
Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/ByteConverter.cs Outdated
Comment thread...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/ByteConverter.cs Outdated
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.String.cs Outdated
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs Outdated
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
steveharter
steveharter reviewed Jul 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
steveharter
steveharter reviewed Jul 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
steveharter
steveharter reviewed Jul 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
steveharter
steveharter reviewed Jul 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonNumberHandling.cs Outdated
steveharter
steveharter reviewed Jul 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.cs Outdated
steveharter
steveharter reviewed Jul 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs Outdated
@layomialayomia changed the title Add JsonNumberHandling & support for (de)serializing numbers from/to stringAdd JsonNumberHandling(Attribute) & support for (de)serializing numbers from/to stringJul 19, 2020
@layomia
layomiaforce-pushed the json_number branch 6 times, most recently from 51b8044 to 3b90de7CompareJuly 20, 2020 08:47
steveharter
steveharter reviewed Jul 20, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/Object.ReadTests.cs
steveharter
steveharter reviewed Jul 20, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
steveharter
steveharter reviewed Jul 20, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
steveharter
steveharter reviewed Jul 20, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
steveharter
steveharter reviewed Jul 20, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
steveharter
steveharter reviewed Jul 20, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
steveharter
steveharter approved these changes Jul 20, 2020
View reviewed changes

@stevehartersteveharter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From other pending changes per offline review, LGTM.

@layomia

layomia commented Jul 21, 2020

Copy link
Copy Markdown
ContributorAuthor

Barring benchmark noise, there are no noticeable perf regressions caused by this PR:

dotnet run --base D:\benchmarks\numbers\master --diff D:\benchmarks\numbers\feature --threshold 2%
summary:
better: 4, geomean: 1.039
worse: 4, geomean: 1.034
total diff: 8
Slowerdiff/baseBase Median (ns)Diff Median (ns)Modality
System.Text.Json.Serialization.Tests.ReadJson.DeserializeFromUtf8Bytes1.0481.4285.00
System.Text.Json.Serialization.Tests.WriteJson.SerializeToUtf8Bytes1.0415760.3716318.96
System.Text.Json.Serialization.Tests.WriteJson.SerializeToUtf8By1.03318.62328.68
System.Text.Json.Serialization.Tests.WriteJson.SerializeToUtf8Bytes1.03120.47123.72
Fasterbase/diffBase Median (ns)Diff Median (ns)Modality
System.Text.Json.Serialization.Tests.WriteJson.SerializeToUtf8Bytes1.04416.34400.33
System.Text.Json.Serialization.Tests.WriteJson.Serializ1.04462715.53445168.06
System.Text.Json.Serialization.Tests.WriteJson.SerializeToUtf8Bytes1.04808.07778.40
System.Text.Json.Serialization.Tests.ReadJson.Deseri1.04304.35293.22

feature.zip
master.zip

@layomialayomia mentioned this pull request Jul 21, 2020
Merged
@layomia
layomia merged commit 650f28f into dotnet:masterJul 21, 2020
@layomia
layomia deleted the json_number branch July 21, 2020 04:13
jozkee
jozkee reviewed Jul 21, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs
Comment thread.../System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IListConverter.cs
Comment thread...ystem.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonNumberHandlingAttribute.cs
Comment thread...ystem.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonNumberHandlingAttribute.cs
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonNumberHandling.cs
Comment thread...libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Helpers.cs
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs Outdated
@layomialayomia mentioned this pull request Jul 21, 2020
Merged
@layomialayomia mentioned this pull request Jul 30, 2020
Closed
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@layomia@Jacksondr5
Add JsonNumberHandling(Attribute) & support for (de)serializing numbe…
48311c0
…rs from/to string (dotnet#39363)
* Add JsonNumberHandling & support for (de)serializing numbers from/to string
* Add JsonNumberHandlingAttribute
* Address review feedback + fixups
* Address review feedback ii
@layomialayomia mentioned this pull request Aug 28, 2020
Merged
ahsonkhan
ahsonkhan reviewed Sep 1, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/JsonConstants.cs
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.cs
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.cs
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs
@layomialayomia mentioned this pull request Sep 15, 2020
Merged
@jozkeejozkee mentioned this pull request Sep 16, 2020
Merged
This was referenced Sep 18, 2020
Closed
Merged
@nathanpovonathanpovo mentioned this pull request Oct 11, 2020
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jozkeejozkeejozkee left review comments
+2 more reviewers
@ahsonkhanahsonkhanahsonkhan left review comments
@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@layomialayomia

Labels

area-System.Text.Jsonnew-api-needs-documentation

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

System.Text.Json: (De)serialization support for quoted numbers

5 participants

@layomia@Dotnet-GitSync-Bot@ahsonkhan@steveharter@jozkee