Re: RFR: 8247781: Day periods support [v15]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 18 additional commits since the last revision: - Merge branch 'master' into dayperiod - Re-worded the spec of appendDayPeriodText, refactored calculation of minute-of-day. - Addressed the following comments: - https://github.com/openjdk/jdk/pull/938#discussion_r522185469 - https://github.com/openjdk/jdk/pull/938#discussion_r522187931 - https://github.com/openjdk/jdk/pull/938#discussion_r522203757 - https://github.com/openjdk/jdk/pull/938#discussion_r522211444 - https://github.com/openjdk/jdk/pull/938#discussion_r522244221 - https://github.com/openjdk/jdk/pull/938#discussion_r522262379 - https://github.com/openjdk/jdk/pull/938#discussion_r522266836 - Added a test case for user defined temporal field resolution with day period. - Clarified 24:00 for "midnight" type in the spec. Some clean up. - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r519061476 - Fixed a comment. - Addressed the following comments: - https://github.com/openjdk/jdk/pull/938#discussion_r518431077 - https://github.com/openjdk/jdk/pull/938#discussion_r518616570 - https://github.com/openjdk/jdk/pull/938#discussion_r518439782 - Fixed typo/grammatical error. - Merge branch 'master' into dayperiod - ... and 8 more: https://git.openjdk.java.net/jdk/compare/0278aec9...e5db226c - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/1aa3134f..e5db226c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=13-14 Stats: 74639 lines in 1037 files changed: 41843 ins; 21966 del; 10830 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v13]
On Thu, 12 Nov 2020 21:49:16 GMT, Stephen Colebourne wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Addressed the following comments: >> - https://github.com/openjdk/jdk/pull/938#discussion_r522185469 >> - https://github.com/openjdk/jdk/pull/938#discussion_r522187931 >> - https://github.com/openjdk/jdk/pull/938#discussion_r522203757 >> - https://github.com/openjdk/jdk/pull/938#discussion_r522211444 >> - https://github.com/openjdk/jdk/pull/938#discussion_r522244221 >> - https://github.com/openjdk/jdk/pull/938#discussion_r522262379 >> - https://github.com/openjdk/jdk/pull/938#discussion_r522266836 > > src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java > line 5063: > >> 5061: } >> 5062: Long moh = context.getValue(MINUTE_OF_HOUR); >> 5063: long value = (hod * 60 + (moh != null ? moh : 0)) % 1_440; > > `long value = Math.floorMod(hod, 24) * 60 + (moh != null ? Math.floorMod(moh, > 60) : 0);` > > and remove the next three lines Thanks, Stephen. I made the changes based on your comments. - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v14]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Re-worded the spec of appendDayPeriodText, refactored calculation of minute-of-day. - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/570b4582..1aa3134f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=12-13 Stats: 18 lines in 1 file changed: 0 ins; 3 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v13]
On Thu, 12 Nov 2020 20:03:14 GMT, Naoto Sato wrote: >> Hi, >> >> Please review the changes for the subject issue. This is to enhance the >> java.time package to support day periods, such as "in the morning", defined >> in CLDR. It will add a new pattern character 'B' and its supporting builder >> method. The motivation and its spec are in this CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254629 >> >> Naoto > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed the following comments: > - https://github.com/openjdk/jdk/pull/938#discussion_r522185469 > - https://github.com/openjdk/jdk/pull/938#discussion_r522187931 > - https://github.com/openjdk/jdk/pull/938#discussion_r522203757 > - https://github.com/openjdk/jdk/pull/938#discussion_r522211444 > - https://github.com/openjdk/jdk/pull/938#discussion_r522244221 > - https://github.com/openjdk/jdk/pull/938#discussion_r522262379 > - https://github.com/openjdk/jdk/pull/938#discussion_r522266836 Approved with one overflow to fix. The spec could do with some rewording too. It might be better to explicitly mention the "resolving phase" with the three parts: > The day period is combined with other fields to make a `LocalTime` in the > resolving phase. If the `HOUR_OF_AMPM` field is present, it is combined with > the day period to make `HOUR_OF_DAY` taking into account any `MINUTE_OF_HOUR` > value. If `HOUR_OF_DAY` is present, it is validated against the day period > taking into account any `MINUTE_OF_HOUR` value. If a day period is present > without `HOUR_OF_DAY`, `MINUTE_OF_HOUR`, `SECOND_OF_MINUTE` and > `NANO_OF_SECOND` then the midpoint of the day period is set as the time. Note that the above is incomplete, and it doesn't describe STRICT/LENIENT, so the actual words will be more complex, src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 5063: > 5061: } > 5062: Long moh = context.getValue(MINUTE_OF_HOUR); > 5063: long value = (hod * 60 + (moh != null ? moh : 0)) % 1_440; `long value = Math.floorMod(hod, 24) * 60 + (moh != null ? Math.floorMod(moh, 60) : 0);` and remove the next three lines - Marked as reviewed by scolebourne (Author). PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v12]
On Thu, 12 Nov 2020 15:21:52 GMT, Roger Riggs wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added a test case for user defined temporal field resolution with day
>> period.
>
> test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
> line 877:
>
>> 875: try {
>> 876: dtf.parse("0 at night");
>> 877: throw new RuntimeException("DateTimeParseException should
>> be thrown");
>
> Testng has `Assert.fail(message)` for this case.
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v12]
On Thu, 12 Nov 2020 15:41:56 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added a test case for user defined temporal field resolution with day
>> period.
>
> src/java.base/share/classes/java/time/format/Parsed.java line 550:
>
>> 548: long midpoint = dayPeriod.mid();
>> 549: fieldValues.put(HOUR_OF_DAY, midpoint / 60);
>> 550: fieldValues.put(MINUTE_OF_HOUR, midpoint % 60);
>
> Set `dayPeriod = null` here, as it has been successfully used.
Fixed.
> src/java.base/share/classes/java/time/format/Parsed.java line 502:
>
>> 500: HOUR_OF_AMPM.checkValidValue(hoap);
>> 501: }
>> 502: if (dayPeriod.includes((hoap % 24 + 12) * 60)) {
>
> Pretty sure the 24 should be 12.
>
> Also, it should use `Math.floorMod()` to handle `HOUR_OF_AMPM = -1` (which is
> 11 o'clock).
>
> Also, this doesn't take into account minutes. So if the day period rule is
> afternoon1 to 18:30 and evening1 after 18:30, and the input is `HOUR_OF_AMPM
> = 6, MINUTE_OF_HOUR = 45`, this will fail to resolve,
>
> Something like this should work (no need for `updateCheckDayPeriodConflict`):
>
> long hoap = fieldValues.remove(HOUR_OF_AMPM);
> if (resolverStyle == ResolverStyle.LENIENT) {
> HOUR_OF_AMPM.checkValidValue(hoap);
> }
> Long mohObj = fieldValues.get(MINUTE_OF_HOUR);
> long moh = mohObj != null ? Math.floorMod(mohObj, 60) : 0;
> long excessHours = dayPeriod.includes(Math.floorMod(hoap, 12) + 12 * 60 +
> moh ? 12 : 0;
> long hod = Math.addExact(hoap, excessHours);
> updateCheckConflict(HOUR_OF_AMPM, HOUR_OF_DAY, hod);
> dayPeriod = null;
Fixed.
> src/java.base/share/classes/java/time/format/Parsed.java line 546:
>
>> 544:
>> 545: // Set the hour-of-day, if not exist and not in STRICT, to
>> the mid point of the day period or am/pm.
>> 546: if (!fieldValues.containsKey(HOUR_OF_DAY) && resolverStyle
>> != ResolverStyle.STRICT) {
>
> Since this logic replaces both `HOUR_OF_DAY` and `MINUTE_OF_HOUR` I think it
> should only be invoked if both do not exist. Beyond that, it doesn't really
> make sense to do this logic if second/sub-second are present. Thus, it needs
> to check all four fields and can then directly set the time.
>
> if (!fieldValues.containsKey(HOUR_OF_DAY) &&
> !fieldValues.containsKey(MINUTE_OF_HOUR) &&
> !fieldValues.containsKey(SECOND_OF_MINUTE) &&
> !fieldValues.containsKey(NANO_OF_SECOND) &&
> resolverStyle != ResolverStyle.STRICT) {
>
> if (dayPeriod != null) {
> long midpoint = dayPeriod.mid();
> resolveTime(midpoint / 60, midpoint % 60, 0, 0);
> dayPeriod = null;
> } else if (fieldValues.containsKey(AMPM_OF_DAY)) {
> long ap = fieldValues.remove(AMPM_OF_DAY);
> if (resolverStyle == ResolverStyle.LENIENT) {
> resolveTime(Math.addExact(Math.multiplyExact(ap, 12), 6), 0, 0, 0);
> } else { // SMART
> AMPM_OF_DAY.checkValidValue(ap);
> resolveTime(ap * 12 + 6, 0, 0, 0);
> }
Fixed.
> src/java.base/share/classes/java/time/format/Parsed.java line 568:
>
>> 566: if (dayPeriod != null) {
>> 567: // Check whether the hod is within the day period
>> 568: updateCheckDayPeriodConflict(HOUR_OF_DAY, hod);
>
> With the other changes, this is the only use of this method and it can
> therefore be simplified (no need to check for conflicts, just whether it
> matches the day period).
Fixed.
> test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
> line 778:
>
>> 776: {"h B", ResolverStyle.SMART, "59 in the morning", 11},
>> 777:
>> 778: {"H B", ResolverStyle.LENIENT, "47 at night", 23},
>
> This test should be split in two - smart (fails) and lenient (succeeds). The
> lenient tests should ensure that
> `p.query(DateTimeFormatter.parsedExcessDays())` returns the expected number
> of excess days.
>
> I'd also add a test for a negative value such as "-2 at night"
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v11]
On Thu, 12 Nov 2020 15:18:44 GMT, Roger Riggs wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Clarified 24:00 for "midnight" type in the spec. Some clean up. > > src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java > line 1483: > >> 1481: * exception is thrown and the day period is ignored. >> 1482: * >> 1483: * "midnight" type allows both "00:00" as the start-of-day and >> "24:00" as the > > Editorial: Add "The " at the beginning of the sentence. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v13]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressed the following comments: - https://github.com/openjdk/jdk/pull/938#discussion_r522185469 - https://github.com/openjdk/jdk/pull/938#discussion_r522187931 - https://github.com/openjdk/jdk/pull/938#discussion_r522203757 - https://github.com/openjdk/jdk/pull/938#discussion_r522211444 - https://github.com/openjdk/jdk/pull/938#discussion_r522244221 - https://github.com/openjdk/jdk/pull/938#discussion_r522262379 - https://github.com/openjdk/jdk/pull/938#discussion_r522266836 - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/a960804f..570b4582 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=11-12 Stats: 75 lines in 3 files changed: 23 ins; 23 del; 29 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v12]
On Tue, 10 Nov 2020 19:52:14 GMT, Naoto Sato wrote:
>> Hi,
>>
>> Please review the changes for the subject issue. This is to enhance the
>> java.time package to support day periods, such as "in the morning", defined
>> in CLDR. It will add a new pattern character 'B' and its supporting builder
>> method. The motivation and its spec are in this CSR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>>
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Added a test case for user defined temporal field resolution with day
> period.
src/java.base/share/classes/java/time/format/Parsed.java line 550:
> 548: long midpoint = dayPeriod.mid();
> 549: fieldValues.put(HOUR_OF_DAY, midpoint / 60);
> 550: fieldValues.put(MINUTE_OF_HOUR, midpoint % 60);
Set `dayPeriod = null` here, as it has been successfully used.
src/java.base/share/classes/java/time/format/Parsed.java line 502:
> 500: HOUR_OF_AMPM.checkValidValue(hoap);
> 501: }
> 502: if (dayPeriod.includes((hoap % 24 + 12) * 60)) {
Pretty sure the 24 should be 12.
Also, it should use `Math.floorMod()` to handle `HOUR_OF_AMPM = -1` (which is
11 o'clock).
Also, this doesn't take into account minutes. So if the day period rule is
afternoon1 to 18:30 and evening1 after 18:30, and the input is `HOUR_OF_AMPM =
6, MINUTE_OF_HOUR = 45`, this will fail to resolve,
Something like this should work (no need for `updateCheckDayPeriodConflict`):
long hoap = fieldValues.remove(HOUR_OF_AMPM);
if (resolverStyle == ResolverStyle.LENIENT) {
HOUR_OF_AMPM.checkValidValue(hoap);
}
Long mohObj = fieldValues.get(MINUTE_OF_HOUR);
long moh = mohObj != null ? Math.floorMod(mohObj, 60) : 0;
long excessHours = dayPeriod.includes(Math.floorMod(hoap, 12) + 12 * 60 +
moh ? 12 : 0;
long hod = Math.addExact(hoap, excessHours);
updateCheckConflict(HOUR_OF_AMPM, HOUR_OF_DAY, hod);
dayPeriod = null;
src/java.base/share/classes/java/time/format/Parsed.java line 546:
> 544:
> 545: // Set the hour-of-day, if not exist and not in STRICT, to
> the mid point of the day period or am/pm.
> 546: if (!fieldValues.containsKey(HOUR_OF_DAY) && resolverStyle
> != ResolverStyle.STRICT) {
Since this logic replaces both `HOUR_OF_DAY` and `MINUTE_OF_HOUR` I think it
should only be invoked if both do not exist. Beyond that, it doesn't really
make sense to do this logic if second/sub-second are present. Thus, it needs to
check all four fields and can then directly set the time.
if (!fieldValues.containsKey(HOUR_OF_DAY) &&
!fieldValues.containsKey(MINUTE_OF_HOUR) &&
!fieldValues.containsKey(SECOND_OF_MINUTE) &&
!fieldValues.containsKey(NANO_OF_SECOND) &&
resolverStyle != ResolverStyle.STRICT) {
if (dayPeriod != null) {
long midpoint = dayPeriod.mid();
resolveTime(midpoint / 60, midpoint % 60, 0, 0);
dayPeriod = null;
} else if (fieldValues.containsKey(AMPM_OF_DAY)) {
long ap = fieldValues.remove(AMPM_OF_DAY);
if (resolverStyle == ResolverStyle.LENIENT) {
resolveTime(Math.addExact(Math.multiplyExact(ap, 12), 6), 0, 0, 0);
} else { // SMART
AMPM_OF_DAY.checkValidValue(ap);
resolveTime(ap * 12 + 6, 0, 0, 0);
}
src/java.base/share/classes/java/time/format/Parsed.java line 568:
> 566: if (dayPeriod != null) {
> 567: // Check whether the hod is within the day period
> 568: updateCheckDayPeriodConflict(HOUR_OF_DAY, hod);
With the other changes, this is the only use of this method and it can
therefore be simplified (no need to check for conflicts, just whether it
matches the day period).
test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line
778:
> 776: {"h B", ResolverStyle.SMART, "59 in the morning", 11},
> 777:
> 778: {"H B", ResolverStyle.LENIENT, "47 at night", 23},
This test should be split in two - smart (fails) and lenient (succeeds). The
lenient tests should ensure that
`p.query(DateTimeFormatter.parsedExcessDays())` returns the expected number of
excess days.
I'd also add a test for a negative value such as "-2 at night"
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v12]
On Tue, 10 Nov 2020 19:52:14 GMT, Naoto Sato wrote: >> Hi, >> >> Please review the changes for the subject issue. This is to enhance the >> java.time package to support day periods, such as "in the morning", defined >> in CLDR. It will add a new pattern character 'B' and its supporting builder >> method. The motivation and its spec are in this CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254629 >> >> Naoto > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Added a test case for user defined temporal field resolution with day > period. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v11]
On Tue, 10 Nov 2020 00:07:16 GMT, Naoto Sato wrote: >> Hi, >> >> Please review the changes for the subject issue. This is to enhance the >> java.time package to support day periods, such as "in the morning", defined >> in CLDR. It will add a new pattern character 'B' and its supporting builder >> method. The motivation and its spec are in this CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254629 >> >> Naoto > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Clarified 24:00 for "midnight" type in the spec. Some clean up. src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 1483: > 1481: * exception is thrown and the day period is ignored. > 1482: * > 1483: * "midnight" type allows both "00:00" as the start-of-day and > "24:00" as the Editorial: Add "The " at the beginning of the sentence. - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v12]
On Tue, 10 Nov 2020 19:52:14 GMT, Naoto Sato wrote:
>> Hi,
>>
>> Please review the changes for the subject issue. This is to enhance the
>> java.time package to support day periods, such as "in the morning", defined
>> in CLDR. It will add a new pattern character 'B' and its supporting builder
>> method. The motivation and its spec are in this CSR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>>
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Added a test case for user defined temporal field resolution with day
> period.
test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line
877:
> 875: try {
> 876: dtf.parse("0 at night");
> 877: throw new RuntimeException("DateTimeParseException should be
> thrown");
Testng has `Assert.fail(message)` for this case.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v12]
On Mon, 9 Nov 2020 01:37:39 GMT, Naoto Sato wrote: >> I've had a look tonight, but found two more problems! >> >> The comments at the start of `resolveTimeLenient()` indicate that setting >> the midpoint in `resolveTimeFields()` is a bad idea - it needs to be done >> inside `resolveTimeLenient()`. (This ensures user-defined fields can resolve >> to ChronoFields IIRC). Thus, the day period resolving would have to be split >> between the two methods. How important is the midpoint logic? Could it just >> be dropped? >> >> Secondly, we need to ensure that "24:00 midnight" (using HOUR_OF_DAY only) >> correctly parses to the end of day midnight, not the start of day or an >> exception. This is related to the problem above. >> >> I _think_ (but haven't confirmed everything yet) that the only thing that >> should be in `resolveTimeFields()` is the resolution of HOUR_OF_AMPM + >> dayPeriod to HOUR_OF_DAY (with `dayPeriod` then being set to `null`). >> Everything else should go in `resolveTimeLenient()` - the midpoint logic in >> the first if block (where time fields are defaulted), and the validation >> logic at the end of the method. > > Thanks for the insights. I believe I fixed both of the issues with the new > commit. Added a test case with the latest commit: https://github.com/openjdk/jdk/commit/a960804ff4d3f7df18e51fe59dcdcaf04542e10a - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v12]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added a test case for user defined temporal field resolution with day period. - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/ccefe70c..a960804f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=10-11 Stats: 76 lines in 1 file changed: 76 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v11]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Clarified 24:00 for "midnight" type in the spec. Some clean up. - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/7140ae27..ccefe70c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=09-10 Stats: 8 lines in 1 file changed: 3 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v10]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressing https://github.com/openjdk/jdk/pull/938#discussion_r519061476 - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/3a9ea64a..7140ae27 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=08-09 Stats: 90 lines in 3 files changed: 56 ins; 34 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v10]
On Fri, 6 Nov 2020 23:59:36 GMT, Stephen Colebourne wrote: >> I find the whole "half way between the start and end" behaviour of day >> periods odd anyway, but if it is to be supported then it should be >> consistent as you've implemented. >> >> Another option I should have thought of earlier would be to simply not >> support the "half way between the start and end" behaviour of LDML in either >> dayPeriod or AM/PM. But since LDML defines it, you've implemented it, and it >> isn't overly harmful I think its OK to leave it in. >> >> Would it be possible for STRICT mode to not have the "half way between the >> start and end" behaviour? > > I've had a look tonight, but found two more problems! > > The comments at the start of `resolveTimeLenient()` indicate that setting the > midpoint in `resolveTimeFields()` is a bad idea - it needs to be done inside > `resolveTimeLenient()`. (This ensures user-defined fields can resolve to > ChronoFields IIRC). Thus, the day period resolving would have to be split > between the two methods. How important is the midpoint logic? Could it just > be dropped? > > Secondly, we need to ensure that "24:00 midnight" (using HOUR_OF_DAY only) > correctly parses to the end of day midnight, not the start of day or an > exception. This is related to the problem above. > > I _think_ (but haven't confirmed everything yet) that the only thing that > should be in `resolveTimeFields()` is the resolution of HOUR_OF_AMPM + > dayPeriod to HOUR_OF_DAY (with `dayPeriod` then being set to `null`). > Everything else should go in `resolveTimeLenient()` - the midpoint logic in > the first if block (where time fields are defaulted), and the validation > logic at the end of the method. Thanks for the insights. I believe I fixed both of the issues with the new commit. - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v9]
On Thu, 5 Nov 2020 23:24:19 GMT, Stephen Colebourne wrote: >> I implemented what you suggested here in the latest PR, but that would be a >> behavioral change which requires a CSR, as "AM" would be resolved to 06:00 >> which was not before. Do you think it would be acceptable? If so, I will >> reopen the CSR and describe the change. (In fact some TCK failed with this >> impl) > > I find the whole "half way between the start and end" behaviour of day > periods odd anyway, but if it is to be supported then it should be consistent > as you've implemented. > > Another option I should have thought of earlier would be to simply not > support the "half way between the start and end" behaviour of LDML in either > dayPeriod or AM/PM. But since LDML defines it, you've implemented it, and it > isn't overly harmful I think its OK to leave it in. > > Would it be possible for STRICT mode to not have the "half way between the > start and end" behaviour? I've had a look tonight, but found two more problems! The comments at the start of `resolveTimeLenient()` indicate that setting the midpoint in `resolveTimeFields()` is a bad idea - it needs to be done inside `resolveTimeLenient()`. (This ensures user-defined fields can resolve to ChronoFields IIRC). Thus, the day period resolving would have to be split between the two methods. How important is the midpoint logic? Could it just be dropped? Secondly, we need to ensure that "24:00 midnight" (using HOUR_OF_DAY only) correctly parses to the end of day midnight, not the start of day or an exception. This is related to the problem above. I _think_ (but haven't confirmed everything yet) that the only thing that should be in `resolveTimeFields()` is the resolution of HOUR_OF_AMPM + dayPeriod to HOUR_OF_DAY (with `dayPeriod` then being set to `null`). Everything else should go in `resolveTimeLenient()` - the midpoint logic in the first if block (where time fields are defaulted), and the validation logic at the end of the method. - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v9]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed a comment. - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/8054ce6b..3a9ea64a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=07-08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Thu, 5 Nov 2020 23:49:25 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed typo/grammatical error.
>
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 5055:
>
>> 5053: @Override
>> 5054: public boolean format(DateTimePrintContext context,
>> StringBuilder buf) {
>> 5055: Long value = context.getValue(MINUTE_OF_DAY);
>
> This does not match the spec: " During formatting, the day period is obtained
> from {@code HOUR_OF_DAY}, and optionally {@code MINUTE_OF_HOUR} if exist"
>
> It is possible and legal to create a Temporal that returns `HOUR_OF_DAY` and
> `MINUTE_OF_HOUR` but not `MINUTE_OF_DAY`. As such, this method must be
> changed to follow the spec.
>
> -
>
> In addition, it is possible for `HOUR_OF_DAY` and `MINUTE_OF_HOUR` to be
> outside their normal bounds. The right behaviour would be to combine the two
> fields within this method, and then use mod to get the value into the range 0
> to 1440 before calling `dayPeriod.include`. (While the fall back behaviour
> below does cover this, it would be better to do what I propose here.)
>
> An example of this is a `TransportTime` class where the day runs from 03:00
> to 27:00 each day (because trains run after midnight for no extra cost to the
> passenger, and it is more convenient for the operator to treat the date that
> way). A `TransportTime` of 26:30 should still resolve to "night1" rather than
> fall back to "am".
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v8]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressed the following comments: - https://github.com/openjdk/jdk/pull/938#discussion_r518431077 - https://github.com/openjdk/jdk/pull/938#discussion_r518616570 - https://github.com/openjdk/jdk/pull/938#discussion_r518439782 - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/b0649899..8054ce6b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=06-07 Stats: 28 lines in 4 files changed: 14 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Fri, 6 Nov 2020 09:12:38 GMT, Stephen Colebourne
wrote:
>> Did you mean in STRICT mode, HOUR_OF_AMPM should default to 0, and to 6 in
>> SMART/LENIENT modes?
>
> No. I mean that when resolving AMPM/dayPeriod in strict mode, and there is no
> HOUR_OF_DAY or HOUR_OF_AMPM, then do not resolve using "half way between"(ie.
> fail). This will produce a result where `LocalTime` cannot be obtained.
>
> var f =
> DateTimeFormatter.ofPattern("B").withResolverStyle(ResolverStyle.STRICT);
> var t = LocalTime.from("at night", f);
> would throw an exception in STRICT mode (whereas SMART or LENIENT would
> return a `LocalTime`). Same with pattern "a".
Changed to throw an exception in STRICT mode.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Fri, 6 Nov 2020 03:00:52 GMT, Naoto Sato wrote:
>> test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line
>> 858:
>>
>>> 856: return new Object[][]{
>>> 857: {STRICT, 0, LocalTime.of(6, 0), 0},
>>> 858: {STRICT, 1, LocalTime.of(18, 0), 1},
>>
>> As mentioned in my other comment, this seems odd in STRICT mode.
>
> Did you mean in STRICT mode, HOUR_OF_AMPM should default to 0, and to 6 in
> SMART/LENIENT modes?
No. I mean that when resolving AMPM/dayPeriod in strict mode, and there is no
HOUR_OF_DAY or HOUR_OF_AMPM, then do not resolve using "half way between"(ie.
fail). This will produce a result where `LocalTime` cannot be obtained.
var f =
DateTimeFormatter.ofPattern("B").withResolverStyle(ResolverStyle.STRICT);
var t = LocalTime.from("at night", f);
would throw an exception in STRICT mode (whereas SMART or LENIENT would return
a `LocalTime`). Same with pattern "a".
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Thu, 5 Nov 2020 23:25:38 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed typo/grammatical error.
>
> test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line
> 858:
>
>> 856: return new Object[][]{
>> 857: {STRICT, 0, LocalTime.of(6, 0), 0},
>> 858: {STRICT, 1, LocalTime.of(18, 0), 1},
>
> As mentioned in my other comment, this seems odd in STRICT mode.
Did you mean in STRICT mode, HOUR_OF_AMPM should default to 0, and to 6 in
SMART/LENIENT modes?
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Thu, 5 Nov 2020 17:12:11 GMT, Naoto Sato wrote:
>> Hi,
>>
>> Please review the changes for the subject issue. This is to enhance the
>> java.time package to support day periods, such as "in the morning", defined
>> in CLDR. It will add a new pattern character 'B' and its supporting builder
>> method. The motivation and its spec are in this CSR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>>
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed typo/grammatical error.
test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line 858:
> 856: return new Object[][]{
> 857: {STRICT, 0, LocalTime.of(6, 0), 0},
> 858: {STRICT, 1, LocalTime.of(18, 0), 1},
As mentioned in my other comment, this seems odd in STRICT mode.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5055:
> 5053: @Override
> 5054: public boolean format(DateTimePrintContext context,
> StringBuilder buf) {
> 5055: Long value = context.getValue(MINUTE_OF_DAY);
This does not match the spec: " During formatting, the day period is obtained
from {@code HOUR_OF_DAY}, and optionally {@code MINUTE_OF_HOUR} if exist"
It is possible and legal to create a Temporal that returns `HOUR_OF_DAY` and
`MINUTE_OF_HOUR` but not `MINUTE_OF_DAY`. As such, this method must be changed
to follow the spec.
-
In addition, it is possible for `HOUR_OF_DAY` and `MINUTE_OF_HOUR` to be
outside their normal bounds. The right behaviour would be to combine the two
fields within this method, and then use mod to get the value into the range 0
to 1440 before calling `dayPeriod.include`. (While the fall back behaviour
below does cover this, it would be better to do what I propose here.)
An example of this is a `TransportTime` class where the day runs from 03:00 to
27:00 each day (because trains run after midnight for no extra cost to the
passenger, and it is more convenient for the operator to treat the date that
way). A `TransportTime` of 26:30 should still resolve to "night1" rather than
fall back to "am".
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Mon, 2 Nov 2020 23:21:22 GMT, Naoto Sato wrote: >> Pulling on this a little more. >> >> As the PR stands, it seems that if the user passes in text with just a >> day-period of "AM" they get a `LocalTime` of 06:00 but if they pass in >> `AMPM_OF_DAY` of "AM" the get no `LocalTime` in the result. Is that right? >> If so, I don't think this is sustainable. >> >> Thats why I think `AMPM_OF_DAY` will have to be resolved to a dayPeriod of >> "am" or "pm". If dayPeriod is more precise than `AMPM_OF_DAY`, then >> dayPeriod can silently take precedence > > I implemented what you suggested here in the latest PR, but that would be a > behavioral change which requires a CSR, as "AM" would be resolved to 06:00 > which was not before. Do you think it would be acceptable? If so, I will > reopen the CSR and describe the change. (In fact some TCK failed with this > impl) I find the whole "half way between the start and end" behaviour of day periods odd anyway, but if it is to be supported then it should be consistent as you've implemented. Another option I should have thought of earlier would be to simply not support the "half way between the start and end" behaviour of LDML in either dayPeriod or AM/PM. But since LDML defines it, you've implemented it, and it isn't overly harmful I think its OK to leave it in. Would it be possible for STRICT mode to not have the "half way between the start and end" behaviour? - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Thu, 5 Nov 2020 17:12:11 GMT, Naoto Sato wrote: >> Hi, >> >> Please review the changes for the subject issue. This is to enhance the >> java.time package to support day periods, such as "in the morning", defined >> in CLDR. It will add a new pattern character 'B' and its supporting builder >> method. The motivation and its spec are in this CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254629 >> >> Naoto > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed typo/grammatical error. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed typo/grammatical error. - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/4aa5b197..b0649899 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=05-06 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Thu, 5 Nov 2020 16:07:30 GMT, Roger Riggs wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed typo/grammatical error.
>
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 1479:
>
>> 1477: * for it in the formatter locale is from 21:00 to 06:00, then
>> {@code HOUR_OF_DAY}
>> 1478: * is set to '1' and {@code MINUTE_OF_HOUR} set to '30'. If {@code
>> AMPM_OF_DAY} exists
>> 1479: * and no {@code HOUR_OF_DAY} is resolved, the parsed day period
>> takes the precedence.
>
> "the precedence" -> "precedence"
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Mon, 2 Nov 2020 19:20:10 GMT, Roger Riggs wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed exception messages. > > src/java.base/share/classes/java/time/format/Parsed.java line 180: > >> 178: cloned.chrono = this.chrono; >> 179: cloned.leapSecond = this.leapSecond; >> 180: cloned.dayPeriod= this.dayPeriod; > > Add space before "=". Fixed. - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 22:03:08 GMT, Naoto Sato wrote: >> Hi, >> >> Please review the changes for the subject issue. This is to enhance the >> java.time package to support day periods, such as "in the morning", defined >> in CLDR. It will add a new pattern character 'B' and its supporting builder >> method. The motivation and its spec are in this CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254629 >> >> Naoto > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed exception messages. Marked as reviewed by rriggs (Reviewer). src/java.base/share/classes/java/time/format/Parsed.java line 180: > 178: cloned.chrono = this.chrono; > 179: cloned.leapSecond = this.leapSecond; > 180: cloned.dayPeriod= this.dayPeriod; Add space before "=". - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v6]
On Wed, 4 Nov 2020 22:13:25 GMT, Naoto Sato wrote:
>> Hi,
>>
>> Please review the changes for the subject issue. This is to enhance the
>> java.time package to support day periods, such as "in the morning", defined
>> in CLDR. It will add a new pattern character 'B' and its supporting builder
>> method. The motivation and its spec are in this CSR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>>
>> Naoto
>
> Naoto Sato has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains ten additional commits since
> the last revision:
>
> - Merge branch 'master' into dayperiod
> - Change based on CSR change.
> - Fixed TCK test failures, added the new pattern char description in
> DateTimeFormatter
> - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516147298
> - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516123190
> - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516138455
> - Fixed exception messages.
> - Addressed the following comments:
>- https://github.com/openjdk/jdk/pull/938#discussion_r515003422
>- https://github.com/openjdk/jdk/pull/938#discussion_r515005296
>- https://github.com/openjdk/jdk/pull/938#discussion_r515008862
>- https://github.com/openjdk/jdk/pull/938#discussion_r515030268
>- https://github.com/openjdk/jdk/pull/938#discussion_r515030880
>- https://github.com/openjdk/jdk/pull/938#discussion_r515032002
>- https://github.com/openjdk/jdk/pull/938#discussion_r515036803
>- https://github.com/openjdk/jdk/pull/938#discussion_r515037626
>- https://github.com/openjdk/jdk/pull/938#discussion_r515038069
>- https://github.com/openjdk/jdk/pull/938#discussion_r515039056
> - 8247781: Day periods support
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
1479:
> 1477: * for it in the formatter locale is from 21:00 to 06:00, then
> {@code HOUR_OF_DAY}
> 1478: * is set to '1' and {@code MINUTE_OF_HOUR} set to '30'. If {@code
> AMPM_OF_DAY} exists
> 1479: * and no {@code HOUR_OF_DAY} is resolved, the parsed day period
> takes the precedence.
"the precedence" -> "precedence"
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v6]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision: - Merge branch 'master' into dayperiod - Change based on CSR change. - Fixed TCK test failures, added the new pattern char description in DateTimeFormatter - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516147298 - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516123190 - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516138455 - Fixed exception messages. - Addressed the following comments: - https://github.com/openjdk/jdk/pull/938#discussion_r515003422 - https://github.com/openjdk/jdk/pull/938#discussion_r515005296 - https://github.com/openjdk/jdk/pull/938#discussion_r515008862 - https://github.com/openjdk/jdk/pull/938#discussion_r515030268 - https://github.com/openjdk/jdk/pull/938#discussion_r515030880 - https://github.com/openjdk/jdk/pull/938#discussion_r515032002 - https://github.com/openjdk/jdk/pull/938#discussion_r515036803 - https://github.com/openjdk/jdk/pull/938#discussion_r515037626 - https://github.com/openjdk/jdk/pull/938#discussion_r515038069 - https://github.com/openjdk/jdk/pull/938#discussion_r515039056 - 8247781: Day periods support - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/e52fe51f..4aa5b197 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=04-05 Stats: 12803 lines in 661 files changed: 7123 ins; 3276 del; 2404 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v5]
On Wed, 4 Nov 2020 18:44:33 GMT, Joe Wang wrote: >> Changes requested by scolebourne (Author). > > Since 24:00 is not represented in for example LocalTime and H (hour-of-day) > 0-23, it seems obvious midnight in the JDK would be 00:00, and the new > DayPeriod tests proves that. The CLDR rule for midnight is also obvious, but > the LDML spec does state that it "strongly recommend" that implementations > provide for the ability to specify whether midnight is supported or not (and > for either 00:00 or 24:00 or both). > > Just wonder whether there is a need to explicitly state the fact about > midnight? DateTimeFormatterBuilder::appendInstant() for example, stated > clearly how 24:00 is processed, e.g. "The end-of-day time of '24:00' is > handled as midnight at the start of the following day." Thanks, Joe. I will incorporate some description about "midnight" in the next update. - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v5]
On Fri, 30 Oct 2020 11:42:49 GMT, Stephen Colebourne wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed TCK test failures, added the new pattern char description in >> DateTimeFormatter > > Changes requested by scolebourne (Author). Since 24:00 is not represented in for example LocalTime and H (hour-of-day) 0-23, it seems obvious midnight in the JDK would be 00:00, and the new DayPeriod tests proves that. The CLDR rule for midnight is also obvious, but the LDML spec does state that it "strongly recommend" that implementations provide for the ability to specify whether midnight is supported or not (and for either 00:00 or 24:00 or both). Just wonder whether there is a need to explicitly state the fact about midnight? DateTimeFormatterBuilder::appendInstant() for example, stated clearly how 24:00 is processed, e.g. "The end-of-day time of '24:00' is handled as midnight at the start of the following day." - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v5]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed TCK test failures, added the new pattern char description in DateTimeFormatter - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/297acc94..e52fe51f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=03-04 Stats: 19 lines in 2 files changed: 1 ins; 0 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v4]
On Mon, 2 Nov 2020 17:08:10 GMT, Stephen Colebourne wrote: >> src/java.base/share/classes/java/time/format/Parsed.java line 497: >> >>> 495: AMPM_OF_DAY.checkValidValue(ap); >>> 496: } >>> 497: updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint >>> / 720); >> >> No need to put `AMPM_OF_DAY` back in here because you've already resolved it >> to `HOUR_OF_DAY` and `MINUTE_OF_HOUR`. There probably does need to be >> validation to check that the day period agrees with the AM/PM value. > > Line can still be removed AFAICT Fixed. - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v4]
On Mon, 2 Nov 2020 17:05:40 GMT, Stephen Colebourne
wrote:
>> MINUTE_OF_HOUR without HOUR_OF_DAY/HOUR_OF_AMPM may not make any sense, so
>> it is only validated in updateCheckDayPeriodConflict.
>
> But can you get a CLDR rule that says "at night" before 05:30 and "In the
> morning" from 05:30 onwards? If you can then I don't think it is handled,
> because HOUR_OF_DAY and MINUTE_OF_HOUR are not used together when checking
> against DayPeriod.
CLDR allows rules that involve MINUTE_OF_HOUR. The validation I meant was
within the `updateCheckDayPeriodConflict`:
long hod = fieldValues.get(HOUR_OF_DAY);
long moh = fieldValues.containsKey(MINUTE_OF_HOUR) ?
fieldValues.get(MINUTE_OF_HOUR) : 0;
long mod = hod * 60 + moh;
if (!dayPeriod.includes(mod)) {
throw new DateTimeException("Conflict found: Resolved time
%02d:%02d".formatted(hod, moh) +
" conflicts with " + dayPeriod);
}
which checks both hod/moh. moh is assumed zero if `MINUTE_OF_HOUR` does not
exist. Are you suggesting `HOUR_OF_DAY` should also be assumed zero if it does
not exist?
>> There are cases where a period crosses midnight, e.g., 23:00-04:00 so it
>> cannot validate am/pm, so I decided to just override ampm with dayperiod
>> without validating.
>
> Pulling on this a little more.
>
> As the PR stands, it seems that if the user passes in text with just a
> day-period of "AM" they get a `LocalTime` of 06:00 but if they pass in
> `AMPM_OF_DAY` of "AM" the get no `LocalTime` in the result. Is that right? If
> so, I don't think this is sustainable.
>
> Thats why I think `AMPM_OF_DAY` will have to be resolved to a dayPeriod of
> "am" or "pm". If dayPeriod is more precise than `AMPM_OF_DAY`, then dayPeriod
> can silently take precedence
I implemented what you suggested here in the latest PR, but that would be a
behavioral change which requires a CSR, as "AM" would be resolved to 06:00
which was not before. Do you think it would be acceptable? If so, I will reopen
the CSR and describe the change. (In fact some TCK failed with this impl)
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v4]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with three additional commits since the last revision: - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516147298 - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516123190 - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516138455 - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/29c47afc..297acc94 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=02-03 Stats: 66 lines in 2 files changed: 53 ins; 3 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Mon, 2 Nov 2020 17:27:35 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed exception messages.
>
> test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
> line 656:
>
>> 654: {"h B", "11 at night", 23},
>> 655: {"h B", "3 at night", 3},
>> 656: {"h B", "11 in the morning", 11},
>
> Need tests for "51 in the morning" (which should parse in LENIENT as "3 in
> the morning" plus 2 days, see how HOUR_OF_DAY=51 works in general.
>
> Similar issue with HOUR_OF_AMPM=3 and AMPM_OF_DAY=4.
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 22:03:08 GMT, Naoto Sato wrote:
>> Hi,
>>
>> Please review the changes for the subject issue. This is to enhance the
>> java.time package to support day periods, such as "in the morning", defined
>> in CLDR. It will add a new pattern character 'B' and its supporting builder
>> method. The motivation and its spec are in this CSR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>>
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed exception messages.
test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line
656:
> 654: {"h B", "11 at night", 23},
> 655: {"h B", "3 at night", 3},
> 656: {"h B", "11 in the morning", 11},
Need tests for "51 in the morning" (which should parse in LENIENT as "3 in the
morning" plus 2 days, see how HOUR_OF_DAY=51 works in general.
Similar issue with HOUR_OF_AMPM=3 and AMPM_OF_DAY=4.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 11:06:59 GMT, Stephen Colebourne wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed exception messages. > > src/java.base/share/classes/java/time/format/Parsed.java line 497: > >> 495: AMPM_OF_DAY.checkValidValue(ap); >> 496: } >> 497: updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint >> / 720); > > No need to put `AMPM_OF_DAY` back in here because you've already resolved it > to `HOUR_OF_DAY` and `MINUTE_OF_HOUR`. There probably does need to be > validation to check that the day period agrees with the AM/PM value. Line can still be removed AFAICT - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 21:30:50 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/time/format/Parsed.java line 472:
>>
>>> 470: }
>>> 471: if (dayPeriod != null) {
>>> 472: if (fieldValues.containsKey(HOUR_OF_DAY)) {
>>
>> Are we certain that the CLDR data does not contain day periods based on
>> minutes as well as hours? This logic does not check against MINUTE_OF_HOUR
>> for example. The logic also conflicts with the spec Javadoc that says
>> MINUTE_OF_HOUR is validated.
>
> MINUTE_OF_HOUR without HOUR_OF_DAY/HOUR_OF_AMPM may not make any sense, so it
> is only validated in updateCheckDayPeriodConflict.
But can you get a CLDR rule that says "at night" before 05:30 and "In the
morning" from 05:30 onwards? If you can then I don't think it is handled,
because HOUR_OF_DAY and MINUTE_OF_HOUR are not used together when checking
against DayPeriod.
>> src/java.base/share/classes/java/time/format/Parsed.java line 500:
>>
>>> 498: }
>>> 499: }
>>> 500: }
>>
>> Looking at the existing logic, the `AMPM_OF_DAY` field is completely ignored
>> if there is no `HOUR_OF_AMPM` field. Thus, there is no validation to check
>> `AMPM_OF_DAY` against `HOUR_OF_DAY`. This seems odd. (AMPM_OF_DAY = 0 and
>> HOUR_OF_DAY=18 does not look like it throws an exception, when it probably
>> should).
>>
>> On solution would be for `AMPM_OF_DAY` to be resolved to a day period at
>> line 427, checking for conflicts with any parsed day period. (a small bug
>> fix behavioural change)
>
> There are cases where a period crosses midnight, e.g., 23:00-04:00 so it
> cannot validate am/pm, so I decided to just override ampm with dayperiod
> without validating.
Pulling on this a little more.
As the PR stands, it seems that if the user passes in text with just a
day-period of "AM" they get a `LocalTime` of 06:00 but if they pass in
`AMPM_OF_DAY` of "AM" the get no `LocalTime` in the result. Is that right? If
so, I don't think this is sustainable.
Thats why I think `AMPM_OF_DAY` will have to be resolved to a dayPeriod of "am"
or "pm". If dayPeriod is more precise than `AMPM_OF_DAY`, then dayPeriod can
silently take precedence
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Fixed exception messages. - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/6e1eade7..29c47afc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v2]
On Fri, 30 Oct 2020 10:33:28 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Addressed the following comments:
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515003422
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515005296
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515008862
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515030268
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515030880
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515032002
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515036803
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515037626
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515038069
>> - https://github.com/openjdk/jdk/pull/938#discussion_r515039056
>
> test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
> line 981:
>
>> 979:
>> 980: {"B", "Text(DayPeriod,SHORT)"},
>> 981: {"BB", "Text(DayPeriod,SHORT)"},
>
> "BB" and "BBB" are not defined to do anything in the CSR. Java should match
> CLDR/LDML rules here.
Fixed.
> test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
> line 540:
>
>> 538: builder.appendDayPeriodText(TextStyle.FULL);
>> 539: DateTimeFormatter f = builder.toFormatter();
>> 540: assertEquals(f.toString(), "Text(DayPeriod,FULL)");
>
> This should be "DayPeriod(FULL)", because an end user might create a
> `TemporalField` with the name "DayPeriod" and the toString should be unique.
Fixed.
> src/java.base/share/classes/java/time/format/Parsed.java line 352:
>
>> 350: (fieldValues.containsKey(MINUTE_OF_HOUR) ?
>> fieldValues.get(MINUTE_OF_HOUR) : 0);
>> 351: if (!dayPeriod.includes(mod)) {
>> 352: throw new DateTimeException("Conflict found: " +
>> changeField + " conflict with day period");
>
> "conflict with day period" -> "conflicts with day period"
>
> Should also include `changeValue` and ideally the valid range
Fixed.
> src/java.base/share/classes/java/time/format/Parsed.java line 472:
>
>> 470: }
>> 471: if (dayPeriod != null) {
>> 472: if (fieldValues.containsKey(HOUR_OF_DAY)) {
>
> Are we certain that the CLDR data does not contain day periods based on
> minutes as well as hours? This logic does not check against MINUTE_OF_HOUR
> for example. The logic also conflicts with the spec Javadoc that says
> MINUTE_OF_HOUR is validated.
MINUTE_OF_HOUR without HOUR_OF_DAY/HOUR_OF_AMPM may not make any sense, so it
is only validated in updateCheckDayPeriodConflict.
> src/java.base/share/classes/java/time/format/Parsed.java line 500:
>
>> 498: }
>> 499: }
>> 500: }
>
> Looking at the existing logic, the `AMPM_OF_DAY` field is completely ignored
> if there is no `HOUR_OF_AMPM` field. Thus, there is no validation to check
> `AMPM_OF_DAY` against `HOUR_OF_DAY`. This seems odd. (AMPM_OF_DAY = 0 and
> HOUR_OF_DAY=18 does not look like it throws an exception, when it probably
> should).
>
> On solution would be for `AMPM_OF_DAY` to be resolved to a day period at line
> 427, checking for conflicts with any parsed day period. (a small bug fix
> behavioural change)
There are cases where a period crosses midnight, e.g., 23:00-04:00 so it cannot
validate am/pm, so I decided to just override ampm with dayperiod without
validating.
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 1489:
>
>> 1487: Objects.requireNonNull(style, "style");
>> 1488: if (style != TextStyle.FULL && style != TextStyle.SHORT &&
>> style != TextStyle.NARROW) {
>> 1489: throw new IllegalArgumentException("Style must be either
>> full, short, or narrow");
>
> Nothing in the Javadoc describes this behaviour. It would make more sense to
> map FULL_STANDALONE to FULL and so on and not throw an exception.
Fixed.
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 1869:
>
>> 1867: } else if (cur == 'B') {
>> 1868: switch (count) {
>> 1869: case 1, 2, 3 ->
>> appendDayPeriodText(TextStyle.SHORT);
>
> I think this should be `case 1`. The 2 and 3 are not in the Javadoc, and I
> don't think they are in LDML. I note that patterns G and E do this though, so
> there is precedent.
Fixed.
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 5094:
>
>> 5092: @Override
>> 5093: public String toString() {
>> 5094: return "Text(DayPeriod," + textStyle + ")";
>
> Should be "DayPeriod(FULL)" to avoid possible `toString` clashes with the
> text printer/parser
Fixed.
>
Re: RFR: 8247781: Day periods support [v2]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressed the following comments: - https://github.com/openjdk/jdk/pull/938#discussion_r515003422 - https://github.com/openjdk/jdk/pull/938#discussion_r515005296 - https://github.com/openjdk/jdk/pull/938#discussion_r515008862 - https://github.com/openjdk/jdk/pull/938#discussion_r515030268 - https://github.com/openjdk/jdk/pull/938#discussion_r515030880 - https://github.com/openjdk/jdk/pull/938#discussion_r515032002 - https://github.com/openjdk/jdk/pull/938#discussion_r515036803 - https://github.com/openjdk/jdk/pull/938#discussion_r515037626 - https://github.com/openjdk/jdk/pull/938#discussion_r515038069 - https://github.com/openjdk/jdk/pull/938#discussion_r515039056 - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/7ac5fa03..6e1eade7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=00-01 Stats: 99 lines in 3 files changed: 48 ins; 2 del; 49 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support
On Thu, 29 Oct 2020 15:59:51 GMT, Naoto Sato wrote:
> Hi,
>
> Please review the changes for the subject issue. This is to enhance the
> java.time package to support day periods, such as "in the morning", defined
> in CLDR. It will add a new pattern character 'B' and its supporting builder
> method. The motivation and its spec are in this CSR:
>
> https://bugs.openjdk.java.net/browse/JDK-8254629
>
> Naoto
Changes requested by scolebourne (Author).
test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line
981:
> 979:
> 980: {"B", "Text(DayPeriod,SHORT)"},
> 981: {"BB", "Text(DayPeriod,SHORT)"},
"BB" and "BBB" are not defined to do anything in the CSR. Java should match
CLDR/LDML rules here.
test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line
540:
> 538: builder.appendDayPeriodText(TextStyle.FULL);
> 539: DateTimeFormatter f = builder.toFormatter();
> 540: assertEquals(f.toString(), "Text(DayPeriod,FULL)");
This should be "DayPeriod(FULL)", because an end user might create a
`TemporalField` with the name "DayPeriod" and the toString should be unique.
src/java.base/share/classes/java/time/format/Parsed.java line 352:
> 350: (fieldValues.containsKey(MINUTE_OF_HOUR) ?
> fieldValues.get(MINUTE_OF_HOUR) : 0);
> 351: if (!dayPeriod.includes(mod)) {
> 352: throw new DateTimeException("Conflict found: " +
> changeField + " conflict with day period");
"conflict with day period" -> "conflicts with day period"
Should also include `changeValue` and ideally the valid range
src/java.base/share/classes/java/time/format/Parsed.java line 472:
> 470: }
> 471: if (dayPeriod != null) {
> 472: if (fieldValues.containsKey(HOUR_OF_DAY)) {
Are we certain that the CLDR data does not contain day periods based on minutes
as well as hours? This logic does not check against MINUTE_OF_HOUR for example.
The logic also conflicts with the spec Javadoc that says MINUTE_OF_HOUR is
validated.
src/java.base/share/classes/java/time/format/Parsed.java line 497:
> 495: AMPM_OF_DAY.checkValidValue(ap);
> 496: }
> 497: updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint /
> 720);
No need to put `AMPM_OF_DAY` back in here because you've already resolved it to
`HOUR_OF_DAY` and `MINUTE_OF_HOUR`. There probably does need to be validation
to check that the day period agrees with the AM/PM value.
src/java.base/share/classes/java/time/format/Parsed.java line 500:
> 498: }
> 499: }
> 500: }
Looking at the existing logic, the `AMPM_OF_DAY` field is completely ignored if
there is no `HOUR_OF_AMPM` field. Thus, there is no validation to check
`AMPM_OF_DAY` against `HOUR_OF_DAY`. This seems odd. (AMPM_OF_DAY = 0 and
HOUR_OF_DAY=18 does not look like it throws an exception, when it probably
should).
On solution would be for `AMPM_OF_DAY` to be resolved to a day period at line
427, checking for conflicts with any parsed day period. (a small bug fix
behavioural change)
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
1489:
> 1487: Objects.requireNonNull(style, "style");
> 1488: if (style != TextStyle.FULL && style != TextStyle.SHORT &&
> style != TextStyle.NARROW) {
> 1489: throw new IllegalArgumentException("Style must be either
> full, short, or narrow");
Nothing in the Javadoc describes this behaviour. It would make more sense to
map FULL_STANDALONE to FULL and so on and not throw an exception.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
1869:
> 1867: } else if (cur == 'B') {
> 1868: switch (count) {
> 1869: case 1, 2, 3 ->
> appendDayPeriodText(TextStyle.SHORT);
I think this should be `case 1`. The 2 and 3 are not in the Javadoc, and I
don't think they are in LDML. I note that patterns G and E do this though, so
there is precedent.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5094:
> 5092: @Override
> 5093: public String toString() {
> 5094: return "Text(DayPeriod," + textStyle + ")";
Should be "DayPeriod(FULL)" to avoid possible `toString` clashes with the text
printer/parser
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5153:
> 5151: * minute-of-day of "at" or "from" attribute
> 5152: */
> 5153: private final long from;
Could these three instance variables be `int` ?
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5252:
> 5250:
> .getLocaleResources(CalendarDataUtility.findRegionOverride(l));
> 5251: String dayPeriodRules = lr.getRules()[1];
> 5252: final
RFR: 8247781: Day periods support
Hi, Please review the changes for the subject issue. This is to enhance the java.time package to support day periods, such as "in the morning", defined in CLDR. It will add a new pattern character 'B' and its supporting builder method. The motivation and its spec are in this CSR: https://bugs.openjdk.java.net/browse/JDK-8254629 Naoto - Commit messages: - 8247781: Day periods support Changes: https://git.openjdk.java.net/jdk/pull/938/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=938=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8247781 Stats: 1022 lines in 17 files changed: 838 ins; 87 del; 97 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
