On Thu, 8 Feb 2024 20:37:18 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 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
