Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/6.0] Fix TimeSpan support in sourcegen #62191

Merged
merged 3 commits into from
Jan 8, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 30, 2021

Backport of #62130 to release/6.0

/cc @eiriktsarpalis

Customer Impact

This is a customer reported bug (#62082). System.Text.Json added support for TimeSpan serialization (see #54186), however this was not extended to source generators. As a result, reflection and sourcegen serializers by default diverge in their handling of TimeSpan instances. While it is fairly straightforward for users to work around the issue, we believe this should be addressed in servicing sooner rather than later when we ship the next major release.

Testing

Added integration tests verifying serialization and deserialization support for TimeSpan in source generators.

Risk

Low to minimal. Product code change simply flags TimeSpan as a "known type" thereby defaulting generated code to the built-in converter.

@ghost
Copy link

ghost commented Nov 30, 2021

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #62130 to release/6.0

/cc @eiriktsarpalis

Customer Impact

Testing

Risk

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

area-System.Text.Json

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 6.0.x milestone Nov 30, 2021
@eiriktsarpalis eiriktsarpalis added the Servicing-consider Issue for next servicing release review label Nov 30, 2021
@eiriktsarpalis eiriktsarpalis self-assigned this Nov 30, 2021
@eiriktsarpalis
Copy link
Member

Servicing approved over email.

@eiriktsarpalis eiriktsarpalis added Servicing-approved Approved for servicing release NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) and removed Servicing-consider Issue for next servicing release review labels Dec 1, 2021
@safern safern removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 15, 2021
@safern
Copy link
Member

safern commented Dec 15, 2021

Closing and reopening to get CI to run again.

@safern safern closed this Dec 15, 2021
@safern safern reopened this Dec 15, 2021
@danmoseley
Copy link
Member

@ericstj @eiriktsarpalis do we need to address feedback?

@ericstj
Copy link
Member

ericstj commented Jan 7, 2022

Sure, I can help with that since @eiriktsarpalis is out.

@ericstj
Copy link
Member

ericstj commented Jan 7, 2022

Browser failures of S.T.J are an existing issue. #61524 I upgraded it to a tracking issue.
Other failures are flaky networking tests. I gave them another chance at success.

@danmoseley
Copy link
Member

Remaining failures are #58927

@danmoseley danmoseley merged commit 26b0a12 into release/6.0 Jan 8, 2022
@danmoseley danmoseley deleted the backport/pr-62130-to-release/6.0 branch January 8, 2022 03:35
@ghost ghost locked as resolved and limited conversation to collaborators Feb 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants