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

2020-11-06 Thread Stephen Colebourne
On Thu, 5 Nov 2020 23:24:19 GMT, Stephen Colebourne  
wrote:

>> 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)
>
> I find the whole "half way between the start and end" behaviour of day 
> periods odd anyway, but if it is to be supported then it should be consistent 
> as you've implemented. 
> 
> Another option I should have thought of earlier would be to simply not 
> support the "half way between the start and end" behaviour of LDML in either 
> dayPeriod or AM/PM. But since LDML defines it, you've implemented it, and it 
> isn't overly harmful I think its OK to leave it in.
> 
> Would it be possible for STRICT mode to not have the "half way between the 
> start and end" behaviour?

I've had a look tonight, but found two more problems!

The comments at the start of `resolveTimeLenient()` indicate that setting the 
midpoint in `resolveTimeFields()` is a bad idea - it needs to be done inside 
`resolveTimeLenient()`. (This ensures user-defined fields can resolve to 
ChronoFields IIRC). Thus, the day period resolving would have to be split 
between the two methods. How important is the midpoint logic? Could it just be 
dropped?

Secondly, we need to ensure that "24:00 midnight" (using HOUR_OF_DAY only) 
correctly parses to the end of day midnight, not the start of day or an 
exception. This is related to the problem above. 

I _think_ (but haven't confirmed everything yet) that the only thing that 
should be in `resolveTimeFields()` is the resolution of HOUR_OF_AMPM + 
dayPeriod to HOUR_OF_DAY (with `dayPeriod` then being set to `null`). 
Everything else should go in `resolveTimeLenient()` - the midpoint logic in the 
first if block (where time fields are defaulted), and the validation logic at 
the end of the method.

-

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


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

2020-11-06 Thread 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 one additional 
commit since the last revision:

  Fixed a comment.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/938/files
  - new: https://git.openjdk.java.net/jdk/pull/938/files/8054ce6b..3a9ea64a

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

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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