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

Allow Time in ISO strings when deserializing DateOnly #102627

Closed
wants to merge 1 commit into from

Conversation

marcinjahn
Copy link

@marcinjahn marcinjahn commented May 23, 2024

Tackles #102594

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 23, 2024
Copy link
Contributor

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

@marcinjahn
Copy link
Author

@dotnet-policy-service agree

@@ -625,10 +628,6 @@ public static void DateOnly_Read_Nullable_Tests()
[InlineData("\\t2022-05-10")] // Otherwise valid but has leading whitespace
[InlineData("2022-05-10 ")] // Otherwise valid but has trailing whitespace
// Fail on arbitrary ISO dates
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Fail on arbitrary ISO dates

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Tests are failing because apparently DateOnly.Parse is rejecting full ISO dates. This might be signal that we should not be as permissive. cc @eerhardt @tarekgh for a second opinion.

@marcinjahn
Copy link
Author

Tests are failing because apparently DateOnly.Parse is rejecting full ISO dates. This might be signal that we should not be as permissive. cc @eerhardt @tarekgh for a second opinion.

Yeah, well, the behavior of DateOnly.Parse is rather odd:

image

It accepts the time, unless it's UTC? Weird

@tarekgh
Copy link
Member

tarekgh commented Jun 24, 2024

It accepts the time, unless it's UTC? Weird

It makes sense to allow parsing the date and ignore the time part (why not?) . When having any time zone information in the string, this will not be acceptable because DateOnly is not meant to work with time zones. In same time it will be weird to allow succeeding the parsing with ignoring the zone info as it can have implication on the result. Imagine parsing 2000-01-23T01:23:45+09:0 and ``2000-01-23T01:23:45Z`. That was why intentionally don't allow parsing zone info. Use DateTime[Offset] if want to work with time zones.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants