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

[Naoto Sato]

On Tue, 2 May 2023 18:34:11 GMT, Justin Lu <jlu at openjdk.org> wrote:

>> Please review this PR which adds the method `caseFoldLanguageTag(String languageTag)` to java.util.Locale.
>> 
>> This method case folds a language tag to adhere to _[section 2.1.1. Formatting of Language Tags of RFC5646](https://www.rfc-editor.org/rfc/rfc5646.html#section-2.1)_. This format is defined as _"All subtags, including extension and private use subtags, use lowercase letters with two exceptions: two-letter and four-letter subtags that neither appear at the start of the tag nor occur after singletons.  Such two-letter subtags are all uppercase ... and four-letter subtags are titlecase."_.
>> 
>> 
>> In order to match the behavior of existing language tag related Locale methods, this method matches the 2.1.1 RFC5646 specification with the following **exceptions**:
>> - Will not case fold variant subtags
>> - Will not case fold private use subtags prefixed by "lvariant"
>> 
>> As an example, `caseFoldLanguageTag("ja-kana-jp-x-lvariant-Oracle-JDK-Standard-Edition")` returns _"ja-Kana-JP-x-lvariant-Oracle-JDK-Standard-Edition"_. Further examples can be seen in the test file.
>
> Justin Lu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add distinction between legacy and grandfathered to spec

Looks good. Some minor comments follow:

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.

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.

test/jdk/java/util/Locale/CaseFoldLanguageTagTest.java line 48:

> 46:  * grandfathered, and irregular. For more info, see the following,
> 47:  * <a href="https://www.rfc-editor.org/rfc/rfc5646.html#section-2.1">Tag Syntax</a>).
> 48:  * In addition, the method is tested to ensure that IllegalArgumentException and

`IllformedLocaleException`?

test/jdk/java/util/Locale/CaseFoldLanguageTagTest.java line 55:

> 53:     @ParameterizedTest
> 54:     @MethodSource("wellFormedTags")
> 55:     public void TestWellFormedTags(String tag, String foldedTag) {

Nit: method name should start with a lowercase char

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?

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.

test/jdk/java/util/Locale/CaseFoldLanguageTagTest.java line 72:

> 70:         try {
> 71:             Locale.caseFoldLanguageTag(null);
> 72:             throw new RuntimeException("$$$ Failure: NPE should be thrown when tag is null");

`assertThrows()` can be used here as well

-------------

PR Review: https://git.openjdk.org/jdk/pull/13679#pullrequestreview-1409713684
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1182938431
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1182940287
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1182943088
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1183004084
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1183002010
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1183006073
PR Review Comment: https://git.openjdk.org/jdk/pull/13679#discussion_r1183013804