Skip to content

Add JSON src-gen support for deserializing with parameterized ctors #56354

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Aug 6, 2021
Merged

Add JSON src-gen support for deserializing with parameterized ctors #56354

merged 4 commits into from
Aug 6, 2021

Conversation

@layomia
Copy link
Contributor

@layomia layomia commented Jul 27, 2021

Also fixes #56182. FYI @avdv, @jasond-s

@layomia layomia added this to the 6.0.0 milestone Jul 27, 2021
@layomia layomia self-assigned this Jul 27, 2021
@ghost
Copy link

ghost commented Jul 27, 2021

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

Issue Details

Also fixes #56182. FYI @avdv, @jasond-s

Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json
Milestone: 6.0.0

@ghost
Copy link

ghost commented Jul 27, 2021

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.

layomia
layomia commented Jul 27, 2021
View reviewed changes
eerhardt
eerhardt reviewed Jul 27, 2021
View reviewed changes
eerhardt
eerhardt reviewed Jul 27, 2021
View reviewed changes
eerhardt
eerhardt reviewed Jul 27, 2021
View reviewed changes
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs Outdated
Copy link
Member

@eerhardt eerhardt Jul 27, 2021

Choose a reason for hiding this comment

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

More reasons why it should be a class.

Copy link
Member

@eiriktsarpalis eiriktsarpalis Aug 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I presume this concern is no longer applicable. Should it be changed into a property?

Copy link
Contributor Author

@layomia layomia Aug 6, 2021

Choose a reason for hiding this comment

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

Thanks, will update in new PR to avoid CI reset.

steveharter
steveharter reviewed Jul 27, 2021
View reviewed changes
eiriktsarpalis
eiriktsarpalis reviewed Jul 28, 2021
View reviewed changes
eiriktsarpalis
eiriktsarpalis reviewed Jul 28, 2021
View reviewed changes
@layomia layomia force-pushed the SrcGenParamCtors branch 2 times, most recently from a7ecaf8 to 5b901b4 Compare August 5, 2021 19:30
@layomia
Copy link
Contributor Author

layomia commented Aug 6, 2021 •
edited
Loading

New APIs in this PR are in alignment with what was agreed on in API review - #45448 (comment). Some aspects of API review still need more consideration, but not the stuff in this PR.

eiriktsarpalis
eiriktsarpalis approved these changes Aug 6, 2021
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Have not yet reviewed the entire PR, but approving to unblock @layomia for today. Will circle back for more feedback on Monday.

@layomia layomia merged commit 91021fe into dotnet:main Aug 6, 2021
@MihaZupan MihaZupan mentioned this pull request Aug 6, 2021
Closed
@layomia layomia mentioned this pull request Aug 6, 2021
Closed
@layomia layomia mentioned this pull request Aug 11, 2021
Closed
23 tasks
@jaredpar jaredpar mentioned this pull request Aug 17, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub left review comments

@eerhardt eerhardt eerhardt left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

+1 more reviewer

@steveharter steveharter steveharter left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@layomia layomia

Labels

area-System.Text.Json new-api-needs-documentation

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

JSON deserialization for positional record throws exception w/ Source Generator

5 participants

@layomia @stephentoub @eiriktsarpalis @steveharter @eerhardt