On Tue, 2 May 2023 18:34:11 GMT, Justin Lu <j...@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
