On Thu, 22 Jun 2023 18:47:06 GMT, Naoto Sato <[email protected]> wrote:
>> Justin Lu has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - Method signature is too long
>> - Exceptions in GetInstanceCheck.java are distinct, catch clause should be
>> reserved for the underlying exception
>> - Rename method to be more clear in CaseCheckVariant.java
>> - Method name case in AvailableLocalesTest.java
>
> test/jdk/java/util/Locale/AvailableLocalesTest.java line 46:
>
>> 44: /**
>> 45: * Test that Locale.getAvailableLocales() is non-empty.
>> 46: * Print out the locales.
>
> If you combine two tests into a single one, I'd put the corresponding bug id
> into the method description.
Unless I am mistaken, both bug IDs, 4122700 8282319 are in the combined file.
> test/jdk/java/util/Locale/AvailableLocalesTest.java line 61:
>
>> 59: */
>> 60: @Test
>> 61: public void StreamEqualsArrayTest() {
>
> Method names should start with a lowercase character
Missed that, thank you.
> test/jdk/java/util/Locale/CaseCheckVariant.java line 50:
>
>> 48: @ParameterizedTest
>> 49: @MethodSource("variants")
>> 50: public void variantNormalizationTest(String variant) {
>
> "Normalization" -> "Case"? Locale does not normalize the variant.
That's a better name, fixed.
> test/jdk/java/util/Locale/GetInstanceCheck.java line 79:
>
>> 77: } catch (InvocationTargetException | IllegalAccessException exc)
>> {
>> 78: Throwable cause = exc.getCause();
>> 79: if (exc.getCause() instanceof NullPointerException) {
>
> Should this condition only be meaningful for `InvocationTargetException`?
Yes you're right, that's a good point. Reserved the catch clause to handle
`InvocationTargetException` and moved `IllegalAccessException` to the method
signature.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238940849
PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238940976
PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238941073
PR Review Comment: https://git.openjdk.org/jdk/pull/14609#discussion_r1238942899
