On Tue, 23 Jan 2024 22:13:09 GMT, Justin Lu <j...@openjdk.org> wrote:
>> Archie Cobbs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add @implNote to Javadoc for toPattern().
>
> src/java.base/share/classes/java/text/MessageFormat.java line 1:
>
>> 1: /*
>
> For this file, please also bump the latter copyright year to 2024.
Will do.
> src/java.base/share/classes/java/text/MessageFormat.java line 585:
>
>> 583: if (fmt instanceof DecimalFormat) {
>> 584: result.append(",number");
>> 585: subformatPattern =
>> ((DecimalFormat)fmt).toPattern();
>
> Could use pattern matching `instanceof` here and in the other instances, but
> I understand if you're trying to stay consistent with the existing code.
Agreed! Old habits :) I'll fix.
> test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line
> 56:
>
>> 54:
>> 55: private static final int NUM_RANDOM_TEST_CASES = 1000;
>> 56: private static final int MAX_FORMAT_NESTING = 3;
>
> Can you add a comment defining `MAX_FORMAT_TESTING`, might not be immediately
> understood without further investigation
Will do.
> test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line
> 96:
>
>> 94: @ParameterizedTest
>> 95: @MethodSource("testCases")
>> 96: public void testRoundTrip(MessageFormat format1) {
>
> Can we also include the original String pattern that created format1, to help
> with debugging. I find myself wondering what the original String was.
>
> Since technically, the full round trip is _pattern string -> MessageFormat1
> -> pattern string -> MessageFormat2_
I would have done that but it's not (easily) possible. The `MessageFormat`'s
are created not from format strings, but by piecing together plain text and
sub-`Format` objects manually. This was we are sure what we're dealing with.
Trying to create format strings with multiple levels of nesting from scratch is
too complex for my brain due to all the levels of quoting required.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464104293
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464104138
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464104181
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1464104467
