Re: RFR: 8247781: Day periods support [v7]
On Thu, 5 Nov 2020 23:49:25 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed typo/grammatical error.
>
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 5055:
>
>> 5053: @Override
>> 5054: public boolean format(DateTimePrintContext context,
>> StringBuilder buf) {
>> 5055: Long value = context.getValue(MINUTE_OF_DAY);
>
> This does not match the spec: " During formatting, the day period is obtained
> from {@code HOUR_OF_DAY}, and optionally {@code MINUTE_OF_HOUR} if exist"
>
> It is possible and legal to create a Temporal that returns `HOUR_OF_DAY` and
> `MINUTE_OF_HOUR` but not `MINUTE_OF_DAY`. As such, this method must be
> changed to follow the spec.
>
> -
>
> In addition, it is possible for `HOUR_OF_DAY` and `MINUTE_OF_HOUR` to be
> outside their normal bounds. The right behaviour would be to combine the two
> fields within this method, and then use mod to get the value into the range 0
> to 1440 before calling `dayPeriod.include`. (While the fall back behaviour
> below does cover this, it would be better to do what I propose here.)
>
> An example of this is a `TransportTime` class where the day runs from 03:00
> to 27:00 each day (because trains run after midnight for no extra cost to the
> passenger, and it is more convenient for the operator to treat the date that
> way). A `TransportTime` of 26:30 should still resolve to "night1" rather than
> fall back to "am".
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Fri, 6 Nov 2020 09:12:38 GMT, Stephen Colebourne
wrote:
>> Did you mean in STRICT mode, HOUR_OF_AMPM should default to 0, and to 6 in
>> SMART/LENIENT modes?
>
> No. I mean that when resolving AMPM/dayPeriod in strict mode, and there is no
> HOUR_OF_DAY or HOUR_OF_AMPM, then do not resolve using "half way between"(ie.
> fail). This will produce a result where `LocalTime` cannot be obtained.
>
> var f =
> DateTimeFormatter.ofPattern("B").withResolverStyle(ResolverStyle.STRICT);
> var t = LocalTime.from("at night", f);
> would throw an exception in STRICT mode (whereas SMART or LENIENT would
> return a `LocalTime`). Same with pattern "a".
Changed to throw an exception in STRICT mode.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Fri, 6 Nov 2020 03:00:52 GMT, Naoto Sato wrote:
>> test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line
>> 858:
>>
>>> 856: return new Object[][]{
>>> 857: {STRICT, 0, LocalTime.of(6, 0), 0},
>>> 858: {STRICT, 1, LocalTime.of(18, 0), 1},
>>
>> As mentioned in my other comment, this seems odd in STRICT mode.
>
> Did you mean in STRICT mode, HOUR_OF_AMPM should default to 0, and to 6 in
> SMART/LENIENT modes?
No. I mean that when resolving AMPM/dayPeriod in strict mode, and there is no
HOUR_OF_DAY or HOUR_OF_AMPM, then do not resolve using "half way between"(ie.
fail). This will produce a result where `LocalTime` cannot be obtained.
var f =
DateTimeFormatter.ofPattern("B").withResolverStyle(ResolverStyle.STRICT);
var t = LocalTime.from("at night", f);
would throw an exception in STRICT mode (whereas SMART or LENIENT would return
a `LocalTime`). Same with pattern "a".
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Thu, 5 Nov 2020 23:25:38 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed typo/grammatical error.
>
> test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line
> 858:
>
>> 856: return new Object[][]{
>> 857: {STRICT, 0, LocalTime.of(6, 0), 0},
>> 858: {STRICT, 1, LocalTime.of(18, 0), 1},
>
> As mentioned in my other comment, this seems odd in STRICT mode.
Did you mean in STRICT mode, HOUR_OF_AMPM should default to 0, and to 6 in
SMART/LENIENT modes?
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Thu, 5 Nov 2020 17:12:11 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 typo/grammatical error.
test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line 858:
> 856: return new Object[][]{
> 857: {STRICT, 0, LocalTime.of(6, 0), 0},
> 858: {STRICT, 1, LocalTime.of(18, 0), 1},
As mentioned in my other comment, this seems odd in STRICT mode.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5055:
> 5053: @Override
> 5054: public boolean format(DateTimePrintContext context,
> StringBuilder buf) {
> 5055: Long value = context.getValue(MINUTE_OF_DAY);
This does not match the spec: " During formatting, the day period is obtained
from {@code HOUR_OF_DAY}, and optionally {@code MINUTE_OF_HOUR} if exist"
It is possible and legal to create a Temporal that returns `HOUR_OF_DAY` and
`MINUTE_OF_HOUR` but not `MINUTE_OF_DAY`. As such, this method must be changed
to follow the spec.
-
In addition, it is possible for `HOUR_OF_DAY` and `MINUTE_OF_HOUR` to be
outside their normal bounds. The right behaviour would be to combine the two
fields within this method, and then use mod to get the value into the range 0
to 1440 before calling `dayPeriod.include`. (While the fall back behaviour
below does cover this, it would be better to do what I propose here.)
An example of this is a `TransportTime` class where the day runs from 03:00 to
27:00 each day (because trains run after midnight for no extra cost to the
passenger, and it is more convenient for the operator to treat the date that
way). A `TransportTime` of 26:30 should still resolve to "night1" rather than
fall back to "am".
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Mon, 2 Nov 2020 23:21:22 GMT, Naoto Sato wrote: >> 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) 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? - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
On Thu, 5 Nov 2020 17:12:11 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 typo/grammatical error. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v7]
> 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 typo/grammatical error. - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/4aa5b197..b0649899 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=05-06 Stats: 2 lines in 2 files 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
Re: RFR: 8247781: Day periods support [v7]
On Thu, 5 Nov 2020 16:07:30 GMT, Roger Riggs wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed typo/grammatical error.
>
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 1479:
>
>> 1477: * for it in the formatter locale is from 21:00 to 06:00, then
>> {@code HOUR_OF_DAY}
>> 1478: * is set to '1' and {@code MINUTE_OF_HOUR} set to '30'. If {@code
>> AMPM_OF_DAY} exists
>> 1479: * and no {@code HOUR_OF_DAY} is resolved, the parsed day period
>> takes the precedence.
>
> "the precedence" -> "precedence"
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/938
