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 [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 [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
