On Tue, 6 Feb 2024 14:59:22 GMT, Roger Riggs <rri...@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 <turban...@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
