Re: RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v2]
On Fri, 2 Feb 2024 22:35:17 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> reflect Naoto's comments
>
> src/java.base/share/classes/java/text/MessageFormat.java line 377:
>
>> 375: * Calendar cal = Calendar.getInstance();
>> 376: * cal.set(123 + 1900, 10, 16);
>> 377: * Object[] arg = {cal.getTime()};
>
> Maybe with the following, better aligned with `java.time`'s example?
>
> Object[] arg = {new GregorianCalendar(2023, Calendar.NOVEMBER, 16).getTime()};
Thanks, that's a lot more concise, fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476830875
Re: RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v2]
On Fri, 2 Feb 2024 22:16:30 GMT, Justin Lu wrote:
>> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8319344)
>> which adds MessageFormat pattern support for the following subformats:
>> ListFormat, CompactNumberFormat, and DateTimeFormatter. This change is
>> intended to provide pattern support for the more recently added JDK Format
>> subclasses, as well as improving java.time formatting within i18n. The draft
>> javadoc can be viewed here:
>> https://cr.openjdk.org/~jlu/docs/api/java.base/java/text/MessageFormat.html.
>> Please see the CSR for more in-depth behavioral changes, as well as
>> limitations.
>>
>> The `FormatTypes`: dtf_date, dtf_time, dtf_datetime, pre-defined
>> DateTimeFormatter(s), and list are added.
>> The `FormatStyles`: compact_short, compact_long, or, and unit are added.
>>
>> For example, previously,
>>
>>
>> Object[] args = {LocalDate.of(2023, 11, 16), LocalDate.of(2023, 11, 27)};
>> MessageFormat.format("It was {0,date,full}, now it is {1,date,full}", args);
>>
>>
>> would throw `Exception java.lang.IllegalArgumentException: Cannot format
>> given Object as a Date`
>>
>> Now, a user can call
>>
>>
>> MessageFormat.format("It was {0,dtf_date,full}, now it is
>> {1,dtf_date,full}", args);
>>
>>
>> which returns "It was Thursday, November 16, 2023, now it is Friday,
>> November 17, 2023"
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> reflect Naoto's comments
src/java.base/share/classes/java/text/MessageFormat.java line 377:
> 375: * Calendar cal = Calendar.getInstance();
> 376: * cal.set(123 + 1900, 10, 16);
> 377: * Object[] arg = {cal.getTime()};
Maybe with the following, better aligned with `java.time`'s example?
Object[] arg = {new GregorianCalendar(2023, Calendar.NOVEMBER, 16).getTime()};
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476820208
Re: RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v2]
On Fri, 2 Feb 2024 22:11:17 GMT, Justin Lu wrote:
>> src/java.base/share/classes/java/text/MessageFormat.java line 676:
>>
>>> 674: * java.time.format.DateTimeFormatter}. In addition, {@code
>>> DateTimeFormatter}
>>> 675: * does not implement {@code equals()}, and thus cannot be
>>> represented as a
>>> 676: * pattern. Any "default"/"medium" styles are omitted according to
>>> the specification.
>>
>> Since `DateTimeFormatter` eventually are converted to j.t.Format with
>> `toFormat()` method, wouldn't it be possible to implement this method for
>> those dtf(s)?
>
> We discussed this separately, but will go over again here in case others are
> interested. Since DTF does not implement `equals()`, even if we implement
> `equals()` for the `ClassicFormat`, we would basically still need to
> implement our own `equals()` for a DTF to effectively compare the
> ClassicFormats.
>
> I had also mentioned that we could reference check the pre-defined DTFs, but
> this won't work either actually. This is because we cannot reference check
> the DTFs, but rather the Format adapted DTFs, which mean they are now new
> ClassicFormats every time since we don't store them.
Thanks. I think it is fine.
>> src/java.base/share/classes/java/text/MessageFormat.java line 1904:
>>
>>> 1902: throw new IllegalArgumentException();
>>> 1903: }
>>> 1904: }
>>
>> Same as above.
>
> I would rather use `fromString(String text)` here, as using `Enum.valueOf()`
> does not work as smoothly as `FormatType`, since the `FormatStyle.text` value
> does not always match the `FormatStyle` enum name.
>
> For example, `FormatStyle.DEFAULT` has a `.text` value of "". So I would have
> to convert a "" to "default" to check for `FormatStyle.DEFAULT` via
> `Enum.valueOf()`, but at the same time, this also means that the behavior
> would now change to accept "default" as `FormatStyle.DEFAULT` (which is not
> in the spec).
>
> I could then add extra code to handle the "default" case, but I think at that
> point it would be much too much complexity compared to this `fromString()`
> method
Let's keep this static method then.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476805391
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476805499
Re: RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v2]
On Thu, 1 Feb 2024 19:50:30 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> reflect Naoto's comments
>
> src/java.base/share/classes/java/text/MessageFormat.java line 370:
>
>> 368: *
>> 369: * MessageFormat provides patterns that support both the {@link
>> java.time} package
>> 370: * and the {@link Date java.util.Date} type. Consider the following
>> three examples,
>
> Might read better with "patterns that support the date/time formatters in the
> java.time.format and java.text packages"
Updated as suggested.
> src/java.base/share/classes/java/text/MessageFormat.java line 375:
>
>> 373: * 1) a date {@code FormatType} with a full {@code
>> FormatStyle},
>> 374: * {@snippet lang=java :
>> 375: * Object[] arg = {new Date(2023, 11, 16)};
>
> This is not correct (year and month are wrong), and actually should not be
> used in an example as the 3-arg constructor is deprecated. Use
> `Calendar.getTime()` to obtain a `Date` instead.
oops - should have caught this, replaced with `Calendar.getTime()` as
recommended
> src/java.base/share/classes/java/text/MessageFormat.java line 676:
>
>> 674: * java.time.format.DateTimeFormatter}. In addition, {@code
>> DateTimeFormatter}
>> 675: * does not implement {@code equals()}, and thus cannot be
>> represented as a
>> 676: * pattern. Any "default"/"medium" styles are omitted according to
>> the specification.
>
> Since `DateTimeFormatter` eventually are converted to j.t.Format with
> `toFormat()` method, wouldn't it be possible to implement this method for
> those dtf(s)?
We discussed this separately, but will go over again here in case others are
interested. Since DTF does not implement `equals()`, even if we implement
`equals()` for the `ClassicFormat`, we would basically still need to implement
our own `equals()` for a DTF to effectively compare the ClassicFormats.
I had also mentioned that we could reference check the pre-defined DTFs, but
this won't work either actually. This is because we cannot reference check the
DTFs, but rather the Format adapted DTFs, which mean they are now new
ClassicFormats every time since we don't store them.
> src/java.base/share/classes/java/text/MessageFormat.java line 1868:
>
>> 1866: throw new IllegalArgumentException();
>> 1867: }
>> 1868: }
>
> Can this be replaced with `Enum.valueOf(String)`, after input is normalized?
Good point, changed and also removed the `.text` field from the `FormatType`
enum, as the enum name is 1 to 1 with the input String.
> src/java.base/share/classes/java/text/MessageFormat.java line 1904:
>
>> 1902: throw new IllegalArgumentException();
>> 1903: }
>> 1904: }
>
> Same as above.
I would rather use `fromString(String text)` here, as using `Enum.valueOf()`
does not work as smoothly as `FormatType`, since the `FormatStyle.text` value
does not always match the `FormatStyle` enum name.
For example, `FormatStyle.DEFAULT` has a `.text` value of "". So I would have
to convert a "" to "default" to check for `FormatStyle.DEFAULT` via
`Enum.valueOf()`, but at the same time, this also means that the behavior would
now change to accept "default" as `FormatStyle.DEFAULT` (which is not in the
spec).
I could then add extra code to handle the "default" case, but I think at that
point it would be much too much complexity.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476794026
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476794049
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476794075
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476794095
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476794127
Re: RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v2]
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8319344)
> which adds MessageFormat pattern support for the following subformats:
> ListFormat, CompactNumberFormat, and DateTimeFormatter. This change is
> intended to provide pattern support for the more recently added JDK Format
> subclasses, as well as improving java.time formatting within i18n. The draft
> javadoc can be viewed here:
> https://cr.openjdk.org/~jlu/docs/api/java.base/java/text/MessageFormat.html.
> Please see the CSR for more in-depth behavioral changes, as well as
> limitations.
>
> The `FormatTypes`: dtf_date, dtf_time, dtf_datetime, pre-defined
> DateTimeFormatter(s), and list are added.
> The `FormatStyles`: compact_short, compact_long, or, and unit are added.
>
> For example, previously,
>
>
> Object[] args = {LocalDate.of(2023, 11, 16), LocalDate.of(2023, 11, 27)};
> MessageFormat.format("It was {0,date,full}, now it is {1,date,full}", args);
>
>
> would throw `Exception java.lang.IllegalArgumentException: Cannot format
> given Object as a Date`
>
> Now, a user can call
>
>
> MessageFormat.format("It was {0,dtf_date,full}, now it is {1,dtf_date,full}",
> args);
>
>
> which returns "It was Thursday, November 16, 2023, now it is Friday, November
> 17, 2023"
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
reflect Naoto's comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17663/files
- new: https://git.openjdk.org/jdk/pull/17663/files/cd15b398..264799b0
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17663=01
- incr: https://webrevs.openjdk.org/?repo=jdk=17663=00-01
Stats: 59 lines in 2 files changed: 6 ins; 21 del; 32 mod
Patch: https://git.openjdk.org/jdk/pull/17663.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17663/head:pull/17663
PR: https://git.openjdk.org/jdk/pull/17663
