Skip to content

Clean up JSON source gen APIs #54527

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
layomia merged 3 commits into from
Jun 25, 2021
Merged

Clean up JSON source gen APIs #54527

layomia merged 3 commits into from
Jun 25, 2021

Conversation

@layomia
Copy link
Contributor

@layomia layomia commented Jun 21, 2021

Addresses misc feedback about the source gen APIs. I plan to merge this PR now, then batch the API diff with upcoming additional changes (to address various to-dos from #45448). I'll take the changes to API review before preview 7.

FYI @pranavkm

@layomia layomia added this to the 6.0.0 milestone Jun 21, 2021
@layomia layomia self-assigned this Jun 21, 2021
@layomia layomia requested a review from jozkee as a code owner June 21, 2021 23:22
@ghost
Copy link

ghost commented Jun 21, 2021

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

Issue Details

Addresses misc feedback about the source gen APIs. I plan to merge this PR now, then batch the API diff with upcoming additional changes (to address various to-dos from #45448). I'll take the changes to API review before preview 7.

FYI @pranavkm

Author: layomia
Assignees: layomia
Labels:

area-System.Text.Json
Milestone: 6.0.0

@ghost
Copy link

ghost commented Jun 21, 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 mentioned this pull request Jun 21, 2021
Merged
@layomia layomia added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jun 21, 2021
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jun 21, 2021
@ghost
Copy link

ghost commented Jun 21, 2021

Tagging @dotnet/compat for awareness of the breaking change.

@layomia layomia force-pushed the SrcGenApiChanges branch from e3312d9 to e939506 Compare June 22, 2021 01:12
eerhardt
eerhardt reviewed Jun 22, 2021
View reviewed changes
eerhardt
eerhardt reviewed Jun 22, 2021
View reviewed changes
@layomia layomia force-pushed the SrcGenApiChanges branch from b87231a to 93a1da7 Compare June 25, 2021 18:36
eerhardt
eerhardt approved these changes Jun 25, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the cleanup!

@ghost
Copy link

ghost commented Jun 25, 2021

Hello @layomia!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@layomia
Copy link
Contributor Author

layomia commented Jun 25, 2021

Test failures are unrelated.

@layomia layomia merged commit 118c530 into dotnet:main Jun 25, 2021
@layomia layomia deleted the SrcGenApiChanges branch June 26, 2021 23:03
@halter73 halter73 mentioned this pull request Jun 28, 2021
Merged
@KalleOlaviNiemitalo KalleOlaviNiemitalo mentioned this pull request Jul 24, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jul 27, 2021
@gewarren
Copy link
Contributor

gewarren commented Aug 13, 2021

It looks like this needs a breaking change issue for Preview 7, if I'm reading it correctly.

@layomia
Copy link
Contributor Author

layomia commented Nov 2, 2021

These breaking changes have already hit customers & the APIs have changed further since then - dotnet/docs#26200.

@layomia layomia removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eerhardt eerhardt eerhardt approved these changes

@eiriktsarpalis eiriktsarpalis Awaiting requested review from eiriktsarpalis

@jozkee jozkee Awaiting requested review from jozkee

+1 more reviewer

@steveharter steveharter steveharter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@layomia layomia

Labels

area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a previous release. new-api-needs-documentation

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@layomia @gewarren @steveharter @eerhardt