On Wed, 31 Jan 2024 22:24:14 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" src/java.base/share/classes/java/text/MessageFormat.java line 370: > 368: * > 369: * MessageFormat provides patterns that support both the {@link > java.time} package > 370: * and the {@link Date java.util.Date} type. Consider the following > three examples, Might read better with "patterns that support the date/time formatters in the java.time.format and java.text packages" src/java.base/share/classes/java/text/MessageFormat.java line 375: > 373: * <p>1) a <i>date</i> {@code FormatType} with a <i>full</i> {@code > FormatStyle}, > 374: * {@snippet lang=java : > 375: * Object[] arg = {new Date(2023, 11, 16)}; This is not correct (year and month are wrong), and actually should not be used in an example as the 3-arg constructor is deprecated. Use `Calendar.getTime()` to obtain a `Date` instead. src/java.base/share/classes/java/text/MessageFormat.java line 676: > 674: * java.time.format.DateTimeFormatter}. In addition, {@code > DateTimeFormatter} > 675: * does not implement {@code equals()}, and thus cannot be > represented as a > 676: * pattern. Any "default"/"medium" styles are omitted according to > the specification. Since `DateTimeFormatter` eventually are converted to j.t.Format with `toFormat()` method, wouldn't it be possible to implement this method for those dtf(s)? src/java.base/share/classes/java/text/MessageFormat.java line 1868: > 1866: throw new IllegalArgumentException(); > 1867: } > 1868: } Can this be replaced with `Enum.valueOf(String)`, after input is normalized? src/java.base/share/classes/java/text/MessageFormat.java line 1904: > 1902: throw new IllegalArgumentException(); > 1903: } > 1904: } Same as above. test/jdk/java/text/Format/MessageFormat/CompactSubFormats.java line 42: > 40: > 41: import static org.junit.jupiter.api.Assertions.assertEquals; > 42: import static org.junit.jupiter.api.Assertions.assertThrows; Does not seem to be used in this test test/jdk/java/text/Format/MessageFormat/CompactSubFormats.java line 62: > 60: @Test > 61: public void badApplyPatternTest() { > 62: // An exception won't be thrown since 'compact_regular' will be > interpreted as a If the test is to ensure the given type is a subformat, the comment and the method name do not seem to represent it. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1475065503 PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1475076366 PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1475100517 PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1475152563 PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1475152764 PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1475153695 PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1475159578
