Re: RFR: 8247781: Day periods support [v3]
On Mon, 2 Nov 2020 19:20:10 GMT, Roger Riggs wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed exception messages. > > src/java.base/share/classes/java/time/format/Parsed.java line 180: > >> 178: cloned.chrono = this.chrono; >> 179: cloned.leapSecond = this.leapSecond; >> 180: cloned.dayPeriod= this.dayPeriod; > > Add space before "=". Fixed. - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 22:03:08 GMT, Naoto Sato wrote: >> 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 exception messages. Marked as reviewed by rriggs (Reviewer). src/java.base/share/classes/java/time/format/Parsed.java line 180: > 178: cloned.chrono = this.chrono; > 179: cloned.leapSecond = this.leapSecond; > 180: cloned.dayPeriod= this.dayPeriod; Add space before "=". - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Mon, 2 Nov 2020 17:27:35 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed exception messages.
>
> test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
> line 656:
>
>> 654: {"h B", "11 at night", 23},
>> 655: {"h B", "3 at night", 3},
>> 656: {"h B", "11 in the morning", 11},
>
> Need tests for "51 in the morning" (which should parse in LENIENT as "3 in
> the morning" plus 2 days, see how HOUR_OF_DAY=51 works in general.
>
> Similar issue with HOUR_OF_AMPM=3 and AMPM_OF_DAY=4.
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 22:03:08 GMT, Naoto Sato wrote:
>> 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 exception messages.
test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line
656:
> 654: {"h B", "11 at night", 23},
> 655: {"h B", "3 at night", 3},
> 656: {"h B", "11 in the morning", 11},
Need tests for "51 in the morning" (which should parse in LENIENT as "3 in the
morning" plus 2 days, see how HOUR_OF_DAY=51 works in general.
Similar issue with HOUR_OF_AMPM=3 and AMPM_OF_DAY=4.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 11:06:59 GMT, Stephen Colebourne wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed exception messages. > > 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 - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 21:30:50 GMT, Naoto Sato 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
Re: RFR: 8247781: Day periods support [v3]
> 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 exception messages. - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/6e1eade7..29c47afc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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
