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

Fri Oct 30 21:43:21 UTC 2020

On Fri, 30 Oct 2020 10:33:28 GMT, Stephen Colebourne <scolebourne at openjdk.org> 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.

> 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` ?

It shares the code with other TempralFields which is Long, so it is more fit to take long imo.

> 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 Map<DayPeriod, Long> pm = new ConcurrentHashMap<>();
> 
> `pm` is a confusing variable name given this method deals with AM/PM concept.

Fixed.

> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 5289:
> 
>> 5287:                 .filter(dp -> dp.getIndex() == index)
>> 5288:                 .findAny()
>> 5289:                 .orElseThrow();
> 
> Isn't is likely that the user could pass in an unexpected `Locale`? If so, then this needs an exception message.

Although it will never throw an exception due to unknown locale (am/pm is always available), I added a DateTimeException w/ message supplier to orElseThrow().

> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 5137:
> 
>> 5135:      * <a href="https://www.unicode.org/reports/tr35/tr35-dates.html#dayPeriods">DayPeriod</a> defined in CLDR.
>> 5136:      */
>> 5137:     static final class DayPeriod {
> 
> Looks like this class could be `ValueBased` as per other PRs

Fixed.

> test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line 712:
> 
>> 710:         }
>> 711:     }
>> 712: 
> 
> There don't seem to be any tests of the resolver logic in `Parsed`.

Test case added.

-------------

PR: https://git.openjdk.java.net/jdk/pull/938