On Thu, 8 Feb 2024 20:37:18 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 one additional > commit since the last revision: > > Move the if-else nFmt checking to a package-private method in NumberFormat
Looks much better. Left some nits. src/java.base/share/classes/java/text/MessageFormat.java line 745: > 743: String nStyle = NumberFormat.matchToStyle(nFmt, locale); > 744: if (nStyle != null) { > 745: return ",number" + (nStyle.equals("") ? nStyle : "," + > nStyle); `nStyle.isEmpty()` may read better src/java.base/share/classes/java/text/MessageFormat.java line 780: > 778: } > 779: } > 780: else if (fmt != null) { This `else if` does not seem necessary ------------- PR Review: https://git.openjdk.org/jdk/pull/17663#pullrequestreview-1871266747 PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1483596204 PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1483599292