On Thu, 29 Oct 2020 15:59:51 GMT, Naoto Sato <na...@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
