On Mon, 5 Feb 2024 23:51:00 GMT, Justin Lu <[email protected]> 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 <[email protected]>
> - 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
