On Mon, 9 Oct 2023 20:13:39 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflect review comments
>
> test/jdk/java/text/Format/NumberFormat/BigDecimalCompatibilityTest.java line
> 162:
>
>> 160: // Check parse and returned value
>> 161: Number parsedValue = assertDoesNotThrow(()->
>> df.parse(longString),
>> 162: "Should not throw a ParseException");
>
> It could catch any exception, so the message could be broader. Maybe just `a
> ParseException` -> `an Exception`.
Thank you for the review. I Adjusted the message, but clarified in the comment
that the original intention was checking for a `ParseException`; fixed in the
other instances as well.
> test/jdk/java/text/Format/NumberFormat/Bug4838107.java line 254:
>
>> 252: "\n\tre-parsed number: " + parsed2 +
>> 253: " (" + parsed2.getClass().getName() + ")" +
>> 254: "\n\tminus sign: " +
>> df.getDecimalFormatSymbols().getMinusSign());
>
> Could use text block to get rid of `\n`/`\t`s, with `.formatted()`. Same for
> the following one.
Adjusted.
> test/jdk/java/text/Format/NumberFormat/Bug8132125.java line 47:
>
>> 45: NumberFormat nf = NumberFormat.getInstance(deCH);
>> 46:
>> 47: // i.e. "\u2019" as decimal separator, "\u2019" as grouping
>> separator
>
> This is incorrect (sorry, I am to be blamed as I wrote this 😄). Should be
> "`\u002E` as decimal separator, " also remove `i.e.` if you move the comment
> on top of the variable declaration
Fixed.
> test/jdk/java/text/Format/NumberFormat/CurrencyFormat.java line 62:
>
>> 60: // currencySymbolsTest() is only ran for COMPAT
>> 61: private static final boolean isCompat =
>> 62: "COMPAT".equals(System.getProperty("java.locale.providers"));
>
> This check could be moved into `currencySymbolsTest()`
I left it as a static declaration as although it is used in
`currencySymbolsTest()`, it is also used in the data provider of
`CurrencyFormatTest()`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16035#discussion_r1355790743
PR Review Comment: https://git.openjdk.org/jdk/pull/16035#discussion_r1355790850
PR Review Comment: https://git.openjdk.org/jdk/pull/16035#discussion_r1355790950
PR Review Comment: https://git.openjdk.org/jdk/pull/16035#discussion_r1355791032
