On Thu, 1 Feb 2024 19:50:30 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> reflect Naoto's comments
>
> 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"
Updated as suggested.
> 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.
oops - should have caught this, replaced with `Calendar.getTime()` as
recommended
> 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)?
We discussed this separately, but will go over again here in case others are
interested. Since DTF does not implement `equals()`, even if we implement
`equals()` for the `ClassicFormat`, we would basically still need to implement
our own `equals()` for a DTF to effectively compare the ClassicFormats.
I had also mentioned that we could reference check the pre-defined DTFs, but
this won't work either actually. This is because we cannot reference check the
DTFs, but rather the Format adapted DTFs, which mean they are now new
ClassicFormats every time since we don't store them.
> 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?
Good point, changed and also removed the `.text` field from the `FormatType`
enum, as the enum name is 1 to 1 with the input String.
> src/java.base/share/classes/java/text/MessageFormat.java line 1904:
>
>> 1902: throw new IllegalArgumentException();
>> 1903: }
>> 1904: }
>
> Same as above.
I would rather use `fromString(String text)` here, as using `Enum.valueOf()`
does not work as smoothly as `FormatType`, since the `FormatStyle.text` value
does not always match the `FormatStyle` enum name.
For example, `FormatStyle.DEFAULT` has a `.text` value of "". So I would have
to convert a "" to "default" to check for `FormatStyle.DEFAULT` via
`Enum.valueOf()`, but at the same time, this also means that the behavior would
now change to accept "default" as `FormatStyle.DEFAULT` (which is not in the
spec).
I could then add extra code to handle the "default" case, but I think at that
point it would be much too much complexity.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476794026
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476794049
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476794075
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476794095
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1476794127
