On Thu, 1 Feb 2024 23:02:59 GMT, Justin Lu <[email protected]> wrote:
>> Archie Cobbs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix misspelling in comment.
>
> test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line
> 26:
>
>> 24: /*
>> 25: * @test
>> 26: * @summary Verify MessageFormat.toPattern() is equivalent to original
>> pattern
>
> Could make the summary a little more specific for future readers
Thanks, should be fixed.
> test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line
> 70:
>
>> 68: @BeforeAll
>> 69: public static void setup() {
>> 70: savedLocale = Locale.getDefault();
>
> I'm not sure we need to save the default locale and restore it, unless I'm
> missing something.
We are verifying output that includes floating point numbers, and the current
locale affects that:
jshell> Locale.setDefault(Locale.US);
jshell> new MessageFormat("{0}").format(new Object[] { 1.23 });
$9 ==> "1.23"
jshell> Locale.setDefault(Locale.FRENCH);
jshell> new MessageFormat("{0}").format(new Object[] { 1.23 });
$11 ==> "1,23"
> test/jdk/java/text/Format/MessageFormat/MessageFormatToPatternTest.java line
> 104:
>
>> 102: Arguments.of("{0,choice,0.0#option A: {0}|1.0#option B:
>> {0}'}'}", "option B: 1.23}"),
>> 103: Arguments.of("{0,choice,0.0#option A: {0}|2.0#option B:
>> {0}'}'}", "option A: 1.23"),
>> 104:
>
> Suggestion:
>
> // Absurd double quote examples
> Arguments.of("Foo '}''''''''}' {0,number,bar'}' '}' } baz ", "Foo }''''} bar}
> } 1 baz "),
> Arguments.of("'''}''{'''}''''}'"), "'}'{'}''}"),
Thanks, should be fixed.
> test/jdk/java/text/Format/MessageFormat/MessageFormatsByArgumentIndex.java
> line 35:
>
>> 33: import java.text.NumberFormat;
>> 34:
>> 35: public class MessageFormatsByArgumentIndex {
>
> Can you bump the latter copyright year for this file
Thanks, should be fixed.
> test/jdk/java/text/Format/MessageFormat/MessageRegression.java line 114:
>
>> 112: * MessageFormat.toPattern has weird rounding behavior.
>> 113: */
>> 114: @Test
>
> This file as well
Thanks, should be fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1475420027
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1475420069
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1475419965
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1475420147
PR Review Comment: https://git.openjdk.org/jdk/pull/17416#discussion_r1475420112
