On Tue, 2 May 2023 18:59:28 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add distinction between legacy and grandfathered to spec
>
> src/java.base/share/classes/java/util/Locale.java line 1809:
>
>> 1807: * <p>Legacy tags with canonical replacements are as follows:
>> 1808: *
>> 1809: * <table class="striped" id="legacy_tags_replacement">
>
> Maybe `id="legacy_tags` attribute added to the above paragraph that starts
> with "This implements..." is clearer.
Thanks for the review and helpful comments, I have addressed this comment and
the other ones you left.
> src/java.base/share/classes/sun/util/locale/LanguageTag.java line 415:
>
>> 413: // Illegal tags
>> 414: if (!parse(tag, null).wellFormed) {
>> 415: throw new IllformedLocaleException("Ill formed tag");
>
> May include additional information by providing `sts` and retrieving the info
> from it.
Good point, I have updated it so that it provides the sts error message with
the exception.
For example,
`Ill formed tag: Invalid subtag: xabadadoo`
> test/jdk/java/util/Locale/CaseFoldLanguageTagTest.java line 56:
>
>> 54: @MethodSource("wellFormedTags")
>> 55: public void TestWellFormedTags(String tag, String foldedTag) {
>> 56: assertEquals(foldedTag, Locale.caseFoldLanguageTag(tag),
>> String.format("Folded %s", tag));
>
> Would it be helpful if both the expected and the result are recorded?
Junit assertEquals provides the actual result in the failure, so no need to
record it.
Example failure:
`org.opentest4j.AssertionFailedError: Folded ABC ==> expected: <ab> but was:
<abc>`
> test/jdk/java/util/Locale/CaseFoldLanguageTagTest.java line 65:
>
>> 63: Locale.caseFoldLanguageTag(tag);
>> 64: throw new RuntimeException("$$$ Failure: ill formed tags
>> should throw IAE");
>> 65: } catch (IllegalArgumentException ignored) {}
>
> `IllformedLocaleException`? Or if the test passes with this version of impl,
> should the test throw an RuntimeException? Also instead of try-catch,
> `assertThrows()` can be used.
The test does not pass like this, I had forgotten to re run the test after
originally switching IAE to IFE in the method itself. (and hence why I did not
catch that it is not updated in the test).
I have switched all the IAE to IFE in the test, and also used assertThrows() as
recommended
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1183074736
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1183074304
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1183074241
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1183074141
