Re: RFR: 8247781: Day periods support [v4]

[Naoto Sato]

On Mon, 2 Nov 2020 17:08:10 GMT, Stephen Colebourne  
wrote:

>> 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.
>
> Line can still be removed AFAICT

Fixed.

-

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

Re: RFR: 8247781: Day periods support [v4]

[Naoto Sato]

On Mon, 2 Nov 2020 17:05:40 GMT, Stephen Colebourne  
wrote:

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

CLDR allows rules that involve MINUTE_OF_HOUR. The validation I meant was 
within the `updateCheckDayPeriodConflict`:
long hod = fieldValues.get(HOUR_OF_DAY);
long moh = fieldValues.containsKey(MINUTE_OF_HOUR) ? 
fieldValues.get(MINUTE_OF_HOUR) : 0;
long mod =  hod * 60 + moh;
if (!dayPeriod.includes(mod)) {
throw new DateTimeException("Conflict found: Resolved time 
%02d:%02d".formatted(hod, moh) +
" conflicts with " + dayPeriod);
}
which checks both hod/moh. moh is assumed zero if `MINUTE_OF_HOUR` does not 
exist. Are you suggesting `HOUR_OF_DAY` should also be assumed zero if it does 
not exist?

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

I implemented what you suggested here in the latest PR, but that would be a 
behavioral change which requires a CSR, as "AM" would be resolved to 06:00 
which was not before. Do you think it would be acceptable? If so, I will reopen 
the CSR and describe the change. (In fact some TCK failed with this impl)

-

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

Re: RFR: 8247781: Day periods support [v4]

[Naoto Sato]

> 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

Naoto Sato has updated the pull request incrementally with three additional 
commits since the last revision:

 - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516147298
 - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516123190
 - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516138455

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/938/files
  - new: https://git.openjdk.java.net/jdk/pull/938/files/29c47afc..297acc94

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=938=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=02-03

  Stats: 66 lines in 2 files changed: 53 ins; 3 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/938.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938

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