RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v4]

[Roger Riggs]

On Mon, 5 Feb 2024 23:51:00 GMT, Justin Lu <jlu at openjdk.org> 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 two additional commits since the last revision:
> 
>  - Apply spacing suggestions
>    
>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>  - add expected message to exception checking

src/java.base/share/classes/java/text/MessageFormat.java line 90:

> 88:  *         dtf_time
> 89:  *         dtf_datetime
> 90:  *         <i>pre-defined DateTimeFormatter(s)</i>

Its not clear why these new options are inserted before the existing formats.
(And also the order of the table below).

src/java.base/share/classes/java/text/MessageFormat.java line 219:

> 217:  *       <th scope="row" style="font-weight:normal" rowspan=1>{@code pre-defined DateTimeFormatter(s)}
> 218:  *       <th scope="row" style="font-weight:normal"><i>(none)</i>
> 219:  *       <td>The {@code pre-defined DateTimeFormatter(s)} are used as a {@code FormatType} | {@link DateTimeFormatter#BASIC_ISO_DATE BASIC_ISO_DATE}, {@link DateTimeFormatter#ISO_LOCAL_DATE ISO_LOCAL_DATE}, {@link DateTimeFormatter#ISO_OFFSET_DATE ISO_OFFSET_DATE}, {@link DateTimeFormatter#ISO_DATE ISO_DATE}, {@link DateTimeFormatter#ISO_LOCAL_TIME ISO_LOCAL_TIME}, {@link DateTimeFormatter#ISO_OFFSET_TIME ISO_OFFSET_TIME}, {@link DateTimeFormatter#ISO_TIME ISO_TIME}, {@link DateTimeFormatter#ISO_LOCAL_DATE_TIME ISO_LOCAL_DATE_TIME}, {@link DateTimeFormatter#ISO_OFFSET_DATE_TIME ISO_OFFSET_DATE_TIME}, {@link DateTimeFormatter#ISO_ZONED_DATE_TIME ISO_ZONED_DATE_TIME}, {@link DateTimeFormatter#ISO_DATE_TIME ISO_DATE_TIME}, {@link DateTimeFormatter#ISO_ORDINAL_DATE ISO_ORDINAL_DATE}, {@link DateTimeFormatter#ISO_WEEK_DATE ISO_WEEK_DATE}, {@link DateTimeFormatter#ISO_INSTANT ISO_INSTANT}, {@link DateTimeFormatter#RFC_1123_DATE_TIME RFC_1123_DATE_TIME}

The "|" is out of place; its not a common delimiter.  Perhaps ":" colon instead.
This source line is too long (80-100 is typical). Breaking the line should not break the table formatting.

src/java.base/share/classes/java/text/MessageFormat.java line 335:

> 333:  * {@code result} returns the following:
> 334:  * <blockquote><pre>
> 335:  * At 12:30 PM on Jul 3, 2053, there was a disturbance in the Force on planet 7.

An explicit date `new Date(7,3,2053)` would give predictable results between the code and the result string.

src/java.base/share/classes/java/text/MessageFormat.java line 681:

> 679:         if (fmt instanceof NumberFormat) {
> 680:             // Add any instances returned from the NumberFormat factory methods
> 681:             if (fmt.equals(NumberFormat.getInstance(locale))) {

This looks like wack-a-mole code, no good design; likely to be hard to maintain.
(I don't have a better idea at the moment though).

src/java.base/share/classes/java/text/MessageFormat.java line 1881:

> 1879:             for (FormatStyle style : values()) {
> 1880:                 // Also check trimmed case-insensitive for historical reasons
> 1881:                 if (text.trim().toLowerCase(Locale.ROOT).equals(style.text)) {

`String.compareIgnoreCase(....)` might be clearer and not need to allocate for a converted string.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1479979607
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1479989358
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1479999184
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1480020800
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1480039800