On Fri, 30 Oct 2020 21:30:50 GMT, Naoto Sato <na...@openjdk.org> wrote:

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

But can you get a CLDR rule that says "at night" before 05:30 and "In the 
morning" from 05:30 onwards? If you can then I don't think it is handled, 
because HOUR_OF_DAY and MINUTE_OF_HOUR are not used together when checking 
against DayPeriod.

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

Pulling on this a little more.

As the PR stands, it seems that if the user passes in text with just a 
day-period of "AM" they get a `LocalTime` of 06:00 but if they pass in 
`AMPM_OF_DAY` of "AM" the get no `LocalTime` in the result. Is that right? If 
so, I don't think this is sustainable.

Thats why I think `AMPM_OF_DAY` will have to be resolved to a dayPeriod of "am" 
or "pm". If dayPeriod is more precise than `AMPM_OF_DAY`, then dayPeriod can 
silently take precedence

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

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

Reply via email to