Skip to content

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

Merged
layomia merged 4 commits into
dotnet:mainfrom
layomia:SrcGenParamCtors
Aug 6, 2021
Merged

Add JSON src-gen support for deserializing with parameterized ctors#56354
layomia merged 4 commits into
dotnet:mainfrom
layomia:SrcGenParamCtors

Conversation

@layomia

@layomialayomia commented Jul 27, 2021

Copy link
Copy Markdown
Contributor

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

@layomialayomia added this to the 6.0.0 milestone Jul 27, 2021
@layomialayomia self-assigned this Jul 27, 2021
@ghost

ghost commented Jul 27, 2021

Copy link
Copy Markdown

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

ghost commented Jul 27, 2021

Copy link
Copy Markdown

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
Comment threadeng/Versions.props Outdated
eerhardt
eerhardt reviewed Jul 27, 2021
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/ref/System.Text.Json.cs Outdated
eerhardt
eerhardt reviewed Jul 27, 2021
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/ref/System.Text.Json.cs Outdated
eerhardt
eerhardt reviewed Jul 27, 2021
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs Outdated

@eerhardteerhardtJul 27, 2021

Copy link
Copy Markdown
Member

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.

@eiriktsarpaliseiriktsarpalisAug 6, 2021•
edited
Loading

Copy link
Copy Markdown
Member

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?

@layomialayomiaAug 6, 2021

Copy link
Copy Markdown
ContributorAuthor

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
Comment threadsrc/libraries/System.Text.Json/Common/JsonConstants.cs Outdated
Comment threadsrc/libraries/System.Text.Json/Common/ReflectionExtensions.cs Outdated
Comment threadsrc/libraries/System.Text.Json/Common/ReflectionExtensions.cs Outdated
Comment threadsrc/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs Outdated
Comment threadsrc/libraries/System.Text.Json/gen/Reflection/TypeWrapper.cs Outdated
Comment threadsrc/libraries/System.Text.Json/ref/System.Text.Json.cs Outdated
Comment thread...em.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs Outdated
eiriktsarpalis
eiriktsarpalis reviewed Jul 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/Common/ReflectionExtensions.cs Outdated
eiriktsarpalis
eiriktsarpalis reviewed Jul 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/gen/Resources/xlf/Strings.ja.xlf Outdated
eiriktsarpalis
eiriktsarpalis reviewed Jul 28, 2021
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/Common/ReflectionExtensions.cs Outdated
@layomia
layomiaforce-pushed the SrcGenParamCtors branch 2 times, most recently from a7ecaf8 to 5b901b4CompareAugust 5, 2021 19:30
@layomia

layomia commented Aug 6, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@eiriktsarpaliseiriktsarpalis left a comment

Copy link
Copy Markdown
Member

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:mainAug 6, 2021
@MihaZupanMihaZupan mentioned this pull request Aug 6, 2021
Closed
@layomialayomia mentioned this pull request Aug 6, 2021
Closed
@layomialayomia mentioned this pull request Aug 11, 2021
Closed
23 tasks
@jaredparjaredpar mentioned this pull request Aug 17, 2021
Closed
@ghostghost locked as resolved and limited conversation to collaborators Sep 7, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@eerhardteerhardteerhardt left review comments
@eiriktsarpaliseiriktsarpaliseiriktsarpalis approved these changes
+1 more reviewer
@steveharterstevehartersteveharter left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@layomialayomia

Labels

area-System.Text.Jsonnew-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