<i18n dev> RFR: 8159337: Introduce a method in Locale class to return the language tags as per RFC 5646 convention [v3]

[Justin Lu]

On Tue, 2 May 2023 18:59:28 GMT, Naoto Sato <naoto at 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