On Mon, 5 Feb 2024 23:51:00 GMT, Justin Lu <j...@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 <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). 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