-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Support ISO 8601 24:00 (end-of-day) in DateTime/DateTimeOffset/TimeOnly parsing #124142
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
base: main
Are you sure you want to change the base?
Conversation
- Modified DateTime.TryCreate to handle hour=24 when minute, second, and millisecond are zero - Modified Utf8Parser.TryCreateDateTime to handle hour=24 with same validation - Added fraction validation in TryParseFormatO and TryParseDateTimeOffsetO - Added comprehensive tests for DateTime, DateTimeOffset, and Utf8Parser - Tests cover year boundaries, leap years, month boundaries, and timezone offsets - Tests verify that hour=24 with non-zero minute/second/fraction is rejected - Tests verify that 9999-12-31T24:00 is rejected (would overflow) Co-authored-by: jeffhandley <1031940+jeffhandley@users.noreply.github.com>
Removed duplicate test cases for non-zero seconds and minutes in ParseExact_Hour24_Invalid tests Co-authored-by: jeffhandley <1031940+jeffhandley@users.noreply.github.com>
...braries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Date.Helpers.cs
Outdated
Show resolved
Hide resolved
jeffhandley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tarekgh This looks pretty reasonable to me; more surgical than I was expecting. What do you think?
...braries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Date.Helpers.cs
Outdated
Show resolved
Hide resolved
Per @tarekgh's feedback, instead of duplicating the date calculation code, we now set hour to 0 when hour=24 and add one day at the end, falling through to the normal code flow. Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
|
@copilot the change here is not enough when parsing using TimeOnly.ParseExact(s, "o"). We should insert the code before the line runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeParse.cs Line 4765 in dee6079
|
|
@copilot need to add tests for TimeOnly.ParseExact cases |
Per @tarekgh's feedback, added hour=24 validation in DoStrictParse before the calendar.TryToDateTime call. This ensures TimeOnly.ParseExact("24:00:00", "HH:mm:ss") works correctly by treating hour=24 as hour=0 and adding one day after the DateTime is created. Added TimeOnly tests to verify hour=24 parsing works and properly rejects invalid cases. Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
Added hour=24 handling in DoStrictParse (commit 175ded8). Now checking if hour=24 before calendar.TryToDateTime call, validating minute/second/fraction are zero, setting hour=0, and adding one day after the DateTime is created. Also added TimeOnly.ParseExact tests to verify the functionality. |
|
@copilot please fix the test failures: [FAIL] System.Tests.TimeOnlyTests.TryParse_Hour24_Success [FAIL] System.Tests.TimeOnlyTests.TryParseExact_Hour24_Success [FAIL] System.Tests.TimeOnlyTests.Parse_Hour24_Success [FAIL] System.Tests.TimeOnlyTests.ParseExact_Hour24_Success |
The fraction field in DateTimeResult and DateTimeRawInfo is initialized to -1 (meaning "not set"). When no fraction is present in the input (e.g., "24:00:00" without ".0000000"), the fraction stays at -1. The check `fraction != 0` incorrectly rejected this because -1 != 0 is true. Changed to `fraction > 0` which correctly allows both -1 (not set) and 0 (explicitly zero), matching the existing pattern used elsewhere in the same methods (e.g., line 2714 and 4842). Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
Fixed in commit ab7028e. The root cause was the fraction field in Changed to |
|
@copilot fix the failures: |
The hour=24 changes make "1997-07-16T24:00" a valid JSON date string. Moved it from InvalidISO8601Tests to ValidISO8601Tests with expected result "1997-07-17T00:00" (next day at midnight). Also added a new invalid test case "1997-07-16T24:01" to verify non-zero minutes are still rejected with hour=24. Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
... Fixed in commit 51f074f. The string |
|
@copilot please fix the failure: |
DateTime.Parse for strings with Z TZD returns DateTimeKind.Local, but the JSON reader returns DateTimeKind.Utc. Following the existing pattern in the file (ValidISO8601TestsWithUtcOffset), moved the Z-suffixed test case to a separate TestingStrings_Hour24_ValidUtc method that uses DateTime.ParseExact with DateTimeStyles.RoundtripKind. Also added DateTimeOffset assertions to both valid test methods. Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
... Fixed in commit 955e82d. The issue was that |
|
/ba-g reported failures are unrelated |
|
@jeffhandley let me know if ok to merge or you want to have a look before doing that. |
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Description
Implements ISO 8601 support for parsing
24:00:00as end-of-day (midnight of next day). Per the ISO 8601 standard, 24:00:00 represents the end of a calendar day and is the same instant as the next day's 00:00:00.Changes Made
Core Library Changes:
DateTime.cs:TryCreate— set hour=0 when hour=24, add day at endDateTimeParse.cs:TryParseFormatO,DoStrictParse,ParseISO8601,ProcessDateTimeSuffix— all handle hour=24Utf8Parser.Date.Helpers.cs:TryCreateDateTime— handle hour=24JsonHelpers.Date.cs:TryCreateDateTime— handle hour=24Test Changes:
ParseExactwithRoundtripKind(matching existing UTC test pattern)Trade-offs
Original prompt
This section details on the original issue you should resolve
<issue_title>Support 24:00 in hh:mm for ISO8601 dates</issue_title>
<issue_description>A time of 24:00 is a valid in ISO8601 dates.
https://en.wikipedia.org/wiki/ISO_8601
Should be fixed in DateTimeOffset.Parse/DateTime.Parse and new Utf8Parser API.</issue_description>
<agent_instructions>Study this issue carefully, including all of the comments and the links to specifications. There is high risk of regression and unintended consequences with this change, so ample test coverage is expected including across different cultures and on various boundary cases. The PR that proposes a fix should cite the trade-offs involved with considering the fix.</agent_instructions>
Comments on the Issue (you are @copilot in this section)
@JeremyKuhne According to ISO 8601-1:2019 "5.3.2 - Beginning of the day" this is explicitly **not** supported:Additionally, "4.3.8 Clock hour and hours duration" is very explicit about valid hours being between
00and23.ISO 8601-2:2019 "7.3.2 Beginning of the day" also calls out explicitly that "end of day" is not supported.
(I don't know if Wikipedia is accurate to some older revision of the specification. I only have access to the most current.)
cc: @layomia, @tarekgh </comment_new>
<comment_new>@tarekgh
I agree with @JeremyKuhne too. the hours should be from 0 to 23. looking at https://tools.ietf.org/html/rfc3339#section-5.7 you can also see it is explicit saying in the section 5.7. Restrictions:
In section 5.6. Internet Date/Time Format it mention:
Although Appendix A. ISO 8601 Collected ABNF suggest
time-hour = 2DIGIT ; 00-24but the same appendix saying:If the latest version of ISO8601 (year 2019) is explicit regarding that as @JeremyKuhne mentioned, then we should stick with that and not support 24 to avoid any confusion.
@JeremyKuhne > I think the key thing here is to be able to parse user input.I'm fine with considering parsing it, but not as part of the "O" format.</comment_new>
<comment_new>@tarekgh
That will be more tricky. DateTime non-exact parser has a lot heuristics to detect which part of the date/time string is year, month, day, hour, minute...etc. trying to support 24 as hour will have a lot of disadvantages here and can cause a lot of other breaks.
I would say, either allow 24 in O only or not.</comment_new>
<comment_new>@tarekgh
@KalleOlaviNiemitalo I think your sugge...
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.