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

Json prototype #1

Merged
merged 37 commits into from
Jul 17, 2019
Merged

Json prototype #1

merged 37 commits into from
Jul 17, 2019

Conversation

kasiabulat
Copy link
Owner

I added API for writable Json DOM under src/System/Text/Json/Node with usage scenarios in tests/WritableJsonApiTests.cs. It is not completed yet, the transformation functions from JsonNode to JsonElement still need to be added. I wrote scenarios mainly for collection initialization, looking up specific properties, and accessing and modifying different parts of Json tree.

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

@joperezr
Copy link

using System;

NIT: Missing licensing headers.


Refers to: src/System.Text.Json/tests/WritableJsonApiTests.cs:1 in f68d450. [](commit_id = f68d450, deletion_comment = False)

src/System.Text.Json/ref/System.Text.Json.cs Outdated Show resolved Hide resolved
src/System.Text.Json/ref/System.Text.Json.cs Outdated Show resolved Hide resolved
src/System.Text.Json/ref/System.Text.Json.cs Outdated Show resolved Hide resolved
src/System.Text.Json/ref/System.Text.Json.cs Outdated Show resolved Hide resolved
src/System.Text.Json/ref/System.Text.Json.cs Outdated Show resolved Hide resolved
src/System.Text.Json/ref/System.Text.Json.cs Show resolved Hide resolved
src/System.Text.Json/ref/System.Text.Json.cs Outdated Show resolved Hide resolved
src/System.Text.Json/ref/System.Text.Json.cs Show resolved Hide resolved
Copy link

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Changes look good to me for now. I think that other comments regarding object and array equality can be addressed in a subsequent PR. Let's get this one merged so that we can start seeing the API of transformation between JsonDocument and JsonNode and see how the sceanrios will look like for that.

@kasiabulat kasiabulat merged commit 8013865 into master Jul 17, 2019
@kasiabulat kasiabulat deleted the JsonPrototype branch July 17, 2019 20:33
public bool TryGetObjectProperty(string propertyName, out JsonObject jsonObject) { throw null; }
public JsonArray GetArrayProperty(string propertyName) { throw null; }
public bool TryGetArrayProperty(string propertyName, out JsonArray jsonArray) { throw null; }
public IEnumerable<JsonNode> GetAllProperties(string propertyName) { throw null; }

Choose a reason for hiding this comment

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

Thinking about this more, I'm guessing that JsonObject is just going to use Dictionary<string, JsonNode> internally, so it can't know about the distinction of two properties with the same name.

That just means that when creating it it needs to know if it's doing "first value", "last value", or throw-on-duplicate. (From JsonDocument it would inherit JsonDocument rules; the JsonNode.Parse would be able to define a different behavior for that particular Parse).

Copy link
Owner Author

Choose a reason for hiding this comment

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

To keep this distinction maybe we could use Dictionary<string, List> (or MultiValueDictionary, but it's in Microsoft.Experimental.Collections, so I guess not a good idea to depend on)? But it's if we really wanted to support all possible Jsons.

Choose a reason for hiding this comment

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

That just means that when creating it it needs to know if it's doing "first value", "last value", or throw-on-duplicate

We probably want to support this as an option so callers can decide the semantics. Similar to https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_Linq_DuplicatePropertyNameHandling.htm

kasiabulat added a commit that referenced this pull request Jul 29, 2019
Basic API for Json writable DOM with scenarios including collection initialization, accessing and modifying Json nodes.
public partial class JsonArray : System.Text.Json.JsonNode, System.Collections.Generic.ICollection<System.Text.Json.JsonNode>, System.Collections.Generic.IEnumerable<System.Text.Json.JsonNode>, System.Collections.Generic.IList<System.Text.Json.JsonNode>, System.Collections.Generic.IReadOnlyCollection<System.Text.Json.JsonNode>, System.Collections.Generic.IReadOnlyList<System.Text.Json.JsonNode>, System.Collections.IEnumerable
{
public JsonArray() { }
public JsonArray(System.Collections.Generic.IEnumerable<bool> values) { }

Choose a reason for hiding this comment

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

What about IEnumerable of decimal, DateTime{Offset}, Guid? It seems like we should support all the types that JsonDocument supports.

Maybe even IEnumerable of byte[] for base-64 encoded text.

@kasiabulat
Copy link
Owner Author

I created the issue for my project: https://github.com/dotnet/corefx/issues/39922

@kasiabulat kasiabulat mentioned this pull request Aug 1, 2019
kasiabulat added a commit that referenced this pull request Aug 9, 2019
* Json prototype (#1)

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

* Json prototype - transformation API  (#2)

* transformation API added
* assertions to existing scenarios added

* Json prototype (#1)

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

* Json prototype - transformation API  (#2)

* transformation API added
* assertions to existing scenarios added

* JsonNumber implementation and tests (#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants