-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8364752: java.time.Instant should be able to parse ISO 8601 offsets of the form HH:mm:ss #26708
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: master
Are you sure you want to change the base?
Conversation
👋 Welcome back naoto! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
How about using different patterns for parsing and printing instead? |
Shall we accompany the changes with a set of tests that verifies |
In fact, the current The following change:
will parse hour-only offset for |
I don't think |
Changed the proposed fix to leniently allow all ISO 8601 offsets by using "+HH" as the pattern. |
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.
Thanks for adding the tests. In its current state, it LGTM based on my limited understanding. I'll leave the final approval to those more knowledgeable in this area.
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.
Doesn't copyright year need a bump?
{"2017-01-01T00:00:00.000"}, | ||
{"2017-01-01T00:00:00.000+0"}, | ||
{"2017-01-01T00:00:00.000+0:"}, | ||
{"2017-01-01T00:00:00.000+02:"}, |
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.
It is nice that leniency allows 0200
(i.e., missing white space), but not 02:
or 02:00:
(i.e., trailing separators).
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.
Looks like a good solution to the problem, thanks.
src/java.base/share/classes/java/time/format/DateTimeFormatter.java
Outdated
Show resolved
Hide resolved
|
||
@Test(dataProvider = "valid_instants") | ||
public void test_parse_valid(String instant) { | ||
Instant.parse(instant); |
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.
The test should verify the correct time is parsed.
And include cases where the minute and second are non-zero.
@@ -3887,7 +3889,8 @@ public int parse(DateTimeParseContext context, CharSequence text, int position) | |||
.appendValue(MINUTE_OF_HOUR, 2).appendLiteral(':') | |||
.appendValue(SECOND_OF_MINUTE, 2) | |||
.appendFraction(NANO_OF_SECOND, minDigits, maxDigits, true) | |||
.appendOffsetId() | |||
.parseLenient() | |||
.appendOffset("+HH", "Z") |
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.
The "+HH" pattern is supposed to be ignoring minutes and seconds but it does not appear to. In jshell, I see:
jshell> Instant.parse("2017-01-01T00:00:00.000-02:10:12")
$8 ==> 2017-01-01T02:10:12Z
It would be more consistent with the original pattern to use "+HH:mm:ss"
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.
IIUC, "ignoring minutes and seconds" refers to formatting, i.e, "-02:10:12" only prints "-02" Parsing offsets in lenient mode always parses minute/seconds.
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.
True, but you have to read further into the appendOffset
prose to know that the minutes and sections can be present. Using the full pattern would convey more quickly the full syntax being parsed.
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.
Using the full pattern means the colons may not be optional. The appendOffset
spec reads:
If the specified pattern is "+HH", the presence of colons is determined by whether the character after the hour digits is a colon or not.
Among the supported patterns, only "+HH" (and I believe "+H" too) take the colons optional in lenient mode, i.e, both "+02:00" and "+0200" are allowed, which suits the ISO 8601 offset format.
….java Right. Changing to your suggested wording Co-authored-by: Roger Riggs <[email protected]>
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.
Looks good, thanks for the explanation of the parsing format.
Instant.parse()
is expected to use the offset zone pattern+HH:mm:ss
(as defined byDateTimeFormatterBuilder.appendOffsetId()
), but it fails to parse hour-only offsets such as+02
. This is because the actual implementation uses+HH:MM:ss
as the pattern. While replacing the pattern in the implementation as with the specification would allow hour-only offsets, it would also introduce compatibility issues, i.e., printing would omit the minutes field when it is zero. So, it is preferable to update the specification to match the implementation. A CSR has also been drafted for this change.Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26708/head:pull/26708
$ git checkout pull/26708
Update a local copy of the PR:
$ git checkout pull/26708
$ git pull https://git.openjdk.org/jdk.git pull/26708/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26708
View PR using the GUI difftool:
$ git pr show -t 26708
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26708.diff
Using Webrev
Link to Webrev Comment