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