On Mon, 18 Nov 2024 20:37:27 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> address Chen's review; make other similar changes
>
> src/java.base/share/classes/java/util/Locale.java line 94:
>
>> 92: * <p> A {@code Locale} is composed of the bolded fields described
>> below; note that a
>> 93: * {@code Locale} need not have all such fields. For example, {@link
>> 94: * Locale#ENGLISH Locale.ENGLISH} is only comprised of the
>> <em>language</em> field.
>
> Side remark: in the `Locale` class itself, you can just use `{@link #ENGLISH
> Locale.ENGLISH}` to generate the link.
Thanks for taking a look Chen.
https://github.com/openjdk/jdk/pull/22192/commits/6b5b5cd067a42dbb98033314851255841e0d91fb
should address your comments, and includes other changes that are in line with
your suggestions. The apidiff and javadoc links have also been refreshed.
> Update: Didn't notice you posted apidiff and javadoc beforehand:
> APIDiff:
> https://cr.openjdk.org/~jlu/8341923_apidiff/java.base/java/util/Locale.html
> Javadoc: https://cr.openjdk.org/~jlu/api/java.base/java/util/Locale.html
(Sorry, I should have linked these directly on this PR).
> src/java.base/share/classes/java/util/Locale.java line 120:
>
>> 118: *
>> 119: * <dd> <em>Syntax:</em> Well-formed {@code language} values have the
>> form {@code [a-zA-Z]{2,8}}.
>> 120: * BCP 47 deviation: this is not the full BCP 47 language production,
>> since it excludes
>
> Should we add `<br>` before these deviations for more straightforward
> rendering?
Good point. Went ahead and just prefixed with a `<dd>` instead (for
consistency).
> src/java.base/share/classes/java/util/Locale.java line 500:
>
>> 498: * <p>Because of these issues, it is recommended that apps migrate
>> 499: * away from constructing non-conforming locales and use the
>> 500: * {@link #forLanguageTag} and {@link Locale.Builder} APIs instead.
>
> Same remarks for links, add a override render text to prevent javadoc from
> generating long parameter lists.
I gave the `Locale.of` links an explicit render. I think the `forLanguageTag`
links were fine w/o an explicit render, as long as `{@link
#forLanguageTag(String)}` was used, not `{@link #forLanguageTag}` (so that the
fully qualified class name is omitted).
On a side note, the default render was pretty nasty for the
`Locale.LanguageRange` see tags.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847824296
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847826300
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847831453
