Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Writable Json DOM - JsonNumber implementation and tests #39917

Merged
35 commits merged into from
Aug 9, 2019

Conversation

kasiabulat
Copy link
Contributor

I am working on adding writable Json DOM under System.Text.Json. It will be modifiable, dictionary-backed API to complement the readonly JsonDocument, with type hierarchy consisting of the base type JsonNode. Currently, I have implemented JsonNumber which is supposed to represent Json numeric value. Other derived types which we are planning to add include: JsonString, JsonBoolean, JsonArray and JsonObject.

More scenarios are available in this class:
https://github.com/kasiabulat/corefx/blob/master/src/System.Text.Json/tests/WritableJsonApiTests.cs
(also in files: WritableJsonApiTests.TestAccessingNestedMembers.cs, WritableJsonApiTests.TestModifying.cs and WritableJsonApiTests.TestTransformations.cs)

cc: @joperezr @bartonjs @ericstj @ahsonkhan @terrajobst @JamesNK

kasiabulat and others added 12 commits July 17, 2019 13:19
Basic API for Json writable DOM with scenarios including collection initialization, accessing and modifying Json nodes.
* transformation API added
* assertions to existing scenarios added
Basic API for Json writable DOM with scenarios including collection initialization, accessing and modifying Json nodes.
* transformation API added
* assertions to existing scenarios added
* IEquatables added
* JsonNumber implementation and unit tests added
* pragmas deleted
* review comments included, more tests added
* decimal support added to JsonNumber
* decimal support added to JsonArray and JsonObject
@dnfclas
Copy link

dnfclas commented Jul 31, 2019

CLA assistant check
All CLA requirements met.

@ericstj ericstj requested review from ahsonkhan, joperezr and bartonjs and removed request for bartonjs and joperezr July 31, 2019 17:24
@ericstj
Copy link
Member

ericstj commented Jul 31, 2019

Can you make sure to have an issue tracking the API review?

@kasiabulat
Copy link
Contributor Author

I created an issue: #39922 and added a PR in my fork with specification: kasiabulat#4.

@joperezr
Copy link
Member

We will want that spec checked-in to corefx main fork as well, so do you mind sending another PR to merge that here as well plz?

@ahsonkhan ahsonkhan added this to the 5.0 milestone Jul 31, 2019
@kasiabulat
Copy link
Contributor Author

kasiabulat commented Aug 7, 2019

🎉 CI works! 🎉
Thank you for all of the suggestions and feedback!
There's only one thing left to consider - currently throwing implicit casts, but maybe for now I can just add it to open questions in specification. (?)

@ericstj
Copy link
Member

ericstj commented Aug 7, 2019

but maybe for now I can just add it to open questions in specification. (?)

Or file an issue to track it, whichever you prefer.

Copy link
Member

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Fix NumberStyles.Float usage.

src/System.Text.Json/tests/JsonNumberTests.cs Outdated Show resolved Hide resolved
src/System.Text.Json/tests/JsonNumberTests.cs Show resolved Hide resolved
Copy link
Member

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Other than merge conflict in resx file, good to merge.

@kasiabulat kasiabulat added the auto-merge Automatically merge PR once CI passes. label Aug 8, 2019
@ghost
Copy link

ghost commented Aug 8, 2019

Hello @kasiabulat!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ahsonkhan
Copy link
Member

ahsonkhan commented Aug 9, 2019

The MacOS test failure is unrelated (in System.Net.Http.Functional.Tests): https://github.com/dotnet/corefx/issues/40115
https://helix.dot.net/api/2019-06-17/jobs/7b444b61-7a47-4c52-a600-4ae703d727cd/workitems/System.Net.Http.Functional.Tests/console

@ghost ghost merged commit 5050ee8 into dotnet:master Aug 9, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…x#39917)

* Json prototype (dotnet/corefx#1)

Basic API for Json writable DOM with scenarios including collection initialization, accessing and modifying Json nodes.

* Json prototype - transformation API  (dotnet/corefx#2)

* transformation API added
* assertions to existing scenarios added

* Json prototype (dotnet/corefx#1)

Basic API for Json writable DOM with scenarios including collection initialization, accessing and modifying Json nodes.

* Json prototype - transformation API  (dotnet/corefx#2)

* transformation API added
* assertions to existing scenarios added

* JsonNumber implementation and tests (dotnet/corefx#3)

* IEquatables added
* JsonNumber implementation and unit tests added
* pragmas deleted
* review comments included, more tests added
* decimal support added to JsonNumber
* decimal support added to JsonArray and JsonObject

* all unimplemented classes and methods with accompanying tests removed

* First part of documentation added

* documentation completed

* missing exceptions added

* JsonElement changes removed

* part of the review comments included

* work on review comments

* code refactor

* more decimal tests added using MemberData

* more decimal tests added using MemberData

* more test cases added

* equals summary adjusted, equals tests added

* more Equals tests added, GetHashCode tests added, minor changes

* scientifing notation support added, rational numbers tests fixes

* rational overflow tests added

* ulong maxvalue tests added to rational types

* presision problems fixes

* exception strings fixed

* CI failing fixes (hopefully), review comments included

* missing == tests added to achieve 100% branch coverage

* review comments included

* trailing whitespaces fixes

* equals comments added

* equals object refactored to call quals json number

* merge fix


Commit migrated from dotnet/corefx@5050ee8
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json auto-merge Automatically merge PR once CI passes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants