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&pr=938&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=938&range=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