On Fri, 30 Oct 2020 10:33:28 GMT, Stephen Colebourne <scolebou...@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
