Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[JSON source gen 1/3] Implement APIs needed by JSON source generator#51149

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
layomia merged 7 commits intodotnet:mainfromlayomia:JsonSourceGen
Apr 15, 2021

Conversation

@layomia
Copy link
Contributor

@layomialayomia commentedApr 12, 2021•
edited
Loading

Contributes to#45448.

Note to reviewers

This PR primarily implements theJsonMetadataServices APIs needed by the JSON source generator when generating code in user assemblies. It is essentially a no-op to existing serializer functionality, hence, no test changes are added in this PR. Tests for the new APIs will be included in an upcoming PR that adds the source generator itself.

The changes are being made in chunks to avoid having one huge PR that might be way more difficult to review. The target completion date for PRs in this series is the preview 4 snap.

PR review can start from the third commit "Add JsonMetadataServices and APIs needed by source generator". The first two commits simply perform some refactoring:

  • Rename JsonClassInfo to JsonTypeInfo
  • Move existing metadata types to new Metadata sub-folder/sub-namespace

Upcoming PRs to complete the implementation

I expect all 3 PRs to be open and reviewed concurrently, with reviewers focusing on the goal of each PR in isolation (to the degree possible). Thanks!

@ghost
Copy link

Tagging subscribers to this area:@eiriktsarpalis,@layomia
See info inarea-owners.md if you want to be subscribed.

Issue Details

Contributes to#45448.

This PR primarily implements theJsonMetadataServices APIs needed by the JSON source generator when generating code in user assemblies. It is essentially a no-op to existing serializer functionality, hence, no test changes are added in this PR. Tests for the new APIs will be included in an upcoming PR that adds the source generator itself.

The changes are being made in chunks to avoid having one huge PR that might be way more difficult to review. The target completion date for PRs in this series is the preview 4 snap.

Note to reviewers

PR review can start from the third commit "Add JsonMetadataServices and APIs needed by source generator". The first two commits simply perform some refactoring:

  • Rename JsonClassInfo to JsonTypeInfo
  • Move existing metadata types to new Metadata sub-folder/sub-namespace

Upcoming PRs to complete the implementation

  • PR that adds and tests source generator
    • will also test the APIs added in this PR
    • will also add and test a selection of overloads described in the next bullet
  • PR that adds and tests new overloads toJsonSerializer andSystem.Net.Http.Json methods that take the new metadata classes being added in this PR
Author:layomia
Assignees:layomia
Labels:

area-System.Text.Json

Milestone:6.0.0

@ghost
Copy link

Note regarding thenew-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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It the idea we can cache common property names together on the caller site, instead of here on the callee side which wouldn't know about the duplicated names?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes that's the idea. The source generator/caller will keep track of duplicate property names and resolve them within the callback to return the list ofJsonPropertyInfos. We still detect collisions when putting the infos in the dictionary for deserialization, and throwIOE for those cases.

However, it will be possible for the same property name to be written multiple times if the caller is only serializing (and not deserializing). The source generator will not create metadata that has this behavior, but the user can construct this themselves unless we decide to validate both sides (essentially populating the deserialization cache even though we don't need to)

Copy link
Member

@eiriktsarpaliseiriktsarpalis 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 minor feedback LGTM.

Copy link

@tdykstratdykstra left a comment

Choose a reason for hiding this comment

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

LGTM, just noted a few typos.

@layomialayomia merged commit354ab2a intodotnet:mainApr 15, 2021
@layomialayomia deleted the JsonSourceGen branchApril 15, 2021 02:55
@ghostghost locked asresolvedand limited conversation to collaboratorsMay 21, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@tdykstratdykstratdykstra left review comments

@eiriktsarpaliseiriktsarpaliseiriktsarpalis approved these changes

@safernsafernsafern left review comments

@jozkeejozkeeAwaiting requested review from jozkee

+2 more reviewers

@CoffeeFluxCoffeeFluxCoffeeFlux left review comments

@steveharterstevehartersteveharter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@layomialayomia

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@layomia@tdykstra@eiriktsarpalis@CoffeeFlux@steveharter@safern
[8]ページ先頭
©2009-2025 Movatter.jp