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

[Justin Lu]

On Tue, 6 Feb 2024 14:59:22 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> 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).

I moved these new types before the existing formats because I wanted them

1. To be grouped with the other date/time related formats
2. To appear before the existing the other date/time related formats, as java.time takes precedence over util.Date

However, I can update it so that the new types/table are chronologically ordered; please let me know if you would prefer that.

> 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.

Replaced with a `:` as suggested. Also truncated the lines here as well as in the other new Format entries.

> 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.

Right, replaced with `new GregorianCalendar(2053, Calendar.JULY, 3, 12, 30).getTime()` since the Date constructor is deprecated. Also turns out the existing example output was wrong, so updated that as well.

> 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).

I agree, it was the existing design that caused for example, CompactNumberFormat to not automatically be supported by MessageFormat. A simple alternative would be storing the potential pre-defined NumberFormats in some data structure that we could just iterate through to feel less “whack a mole” like, but that array would still suffer from the same maintenance issues. I’ll try to think of something better.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1480614155
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1480614912
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1480615799
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1480617769