RFR: 8247781: Day periods support

[Stephen Colebourne]

On Thu, 29 Oct 2020 15:59:51 GMT, Naoto Sato <naoto at openjdk.org> 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 Map<DayPeriod, Long> pm = new ConcurrentHashMap<>();

`pm` is a confusing variable name given this method deals with AM/PM concept.

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.

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

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

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

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