<i18n dev> RFR: 8247781: Day periods support [v12]

[Naoto Sato]

On Thu, 12 Nov 2020 15:41:56 GMT, Stephen Colebourne <scolebourne at openjdk.org> 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