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.

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

Fix TimeSpan support in sourcegen #62130

Merged
merged 2 commits into from
Nov 30, 2021
Merged

Fix TimeSpan support in sourcegen #62130

merged 2 commits into from
Nov 30, 2021

Conversation

eiriktsarpalis
Copy link
Member

Brings TimeSpan sourcegen serialization semantics in sync with the reflection-based serializer.

Fix #62082.

@ghost
Copy link

ghost commented Nov 29, 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

Brings TimeSpan sourcegen serialization semantics in sync with the reflection-based serializer.

Fix #62082.

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json
Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Nov 29, 2021
@eiriktsarpalis eiriktsarpalis mentioned this pull request Nov 29, 2021
Closed
@eiriktsarpalis
Copy link
Member Author

@layomia @ericstj should this be considered for servicing?

@ericstj
Copy link
Member

ericstj commented Nov 29, 2021

I think this should go to servicing.
We are silently giving different behavior between source-gen and non-source-gen. Previously I stated it's fine to "not support" something in source gen, but we either need to tell the user that it is not supported or fallback to metadata mode or runtime. Here we are silently giving different results in source gen.

We should fix it before it becomes "behavior" that is harder to change without breaking someone. As it is we should probably file a breaking change with this.

@eiriktsarpalis eiriktsarpalis added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Nov 30, 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 Nov 30, 2021
@ghost
Copy link

ghost commented Nov 30, 2021 •
edited by eiriktsarpalis
Loading

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

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

@eiriktsarpalis eiriktsarpalis merged commit 1962c6c into dotnet:main Nov 30, 2021
@joshpeterson joshpeterson mentioned this pull request Nov 30, 2021
Closed
@eiriktsarpalis
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1521216990

@github-actions github-actions bot mentioned this pull request Nov 30, 2021
Merged
@eiriktsarpalis eiriktsarpalis deleted the fix-timespan-sourcegen branch November 30, 2021 14:11
@eiriktsarpalis eiriktsarpalis 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 30, 2021
@eiriktsarpalis eiriktsarpalis mentioned this pull request Nov 30, 2021
Closed
2 tasks
This was referenced Nov 30, 2021
Closed
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@layomia layomia layomia approved these changes

Assignees
No one assigned
Labels
area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

TimeSpan is (de)serialized incorrectly using the JSON source generator
3 participants
@eiriktsarpalis @ericstj @layomia