Skip to content

[release/6.0] Support JsonConverterFactory with src-gen - #58652

Merged
Anipik merged 4 commits into
release/6.0from
backport/pr-58398-to-release/6.0
Sep 7, 2021
Merged

[release/6.0] Support JsonConverterFactory with src-gen#58652
Anipik merged 4 commits into
release/6.0from
backport/pr-58398-to-release/6.0

Conversation

@github-actions

@github-actionsgithub-actionsBot commented Sep 3, 2021

Copy link
Copy Markdown
Contributor

Backport of #58398 to release/6.0

/cc @steveharter

Customer Impact

Support for JsonConverterFactory is currently broken in source generators. It has the potential to block customers from migrating existing reflection-based serialization DTOs to source gen.

Testing

Added testing that validate the broken scenaria.

Risk

Low to moderate. Accommodating factories requires a moderate amount of refactoring in product code.

@ghost

ghost commented Sep 3, 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

Backport of #58398 to release/6.0

/cc @steveharter

Customer Impact

Testing

Risk

Author:github-actions[bot]
Assignees:-
Labels:

area-System.Text.Json

Milestone:-

@Anipik

Copy link
Copy Markdown
Contributor

cc @danmoseley

@layomialayomia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, source/generated code changes are sound; test coverage is robust and addresses this core scenario (JsonConverterFactory + src-gen) for 6.0. cc @danmoseley

@danmoseley

Copy link
Copy Markdown
Contributor

Given the churn and that it's arguably a documentable limitation, I doubt this would make the RC2 bar (starting 9/14). But I'm OK taking this now as it will increase adoption of this new feature. As discussed offline, I appreciate the comprehensive tests but since there's significant churn here I would prefer to get another pair of eyes reviewing it before merging.

@danmoseley

Copy link
Copy Markdown
Contributor

Oh, I see @layomia did that. OK, we can merge when green.

@danmoseleydanmoseley added the Servicing-approved Approved for servicing release label Sep 7, 2021
@Anipik
Anipik merged commit 3b6f9c6 into release/6.0Sep 7, 2021
@akoeplinger
akoeplinger deleted the backport/pr-58398-to-release/6.0 branch September 9, 2021 11:25
@ghostghost locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Text.JsonServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Anipik@danmoseley@eiriktsarpalis@layomia@steveharter