On Mon, 18 Nov 2024 07:41:31 GMT, Justin Lu <[email protected]> wrote:
> Please review this PR and corresponding CSR which includes a wide range of
> specification improvements for java.util.Locale. See the CSR for further
> detail. Other changes/suggestions are welcomed to be included as part of this
> change. Alternatively the diffs can be viewed on the API diff link attached
> on the CSR.
Would be nice if you can render the APIDiff and javadoc and share on
cr.openjdk.org :)
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.
src/java.base/share/classes/java/util/Locale.java line 95:
> 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.
> 95: * Contrarily, a {@code Locale} such as the one returned by {@code
"Contrarily" is fine, but I think we use "In contrast" more often.
src/java.base/share/classes/java/util/Locale.java line 100:
> 98: * the United States using the Latin alphabet and numerics for use in
> POSIX
> 99: * environments.
> 100: * {@code Locale} implements IETF BCP 47 and any deviations should be
> observed
I recommend a paragraph break here with `<p>`
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?
src/java.base/share/classes/java/util/Locale.java line 210:
> 208: * only checks if an individual field satisfies the syntactic
> 209: * requirement (is well-formed), but does not validate the value
> 210: * itself. Conversely, {@link #of(String, String, String)} and its
> overloads do not
Suggestion:
* itself. Conversely, {@link #of(String, String, String) Locale::of} and its
overloads do not
I think javadoc tends to render those arguments as full-blown
`of(java.lang.String, java.lang.String, java.lang.String)`, which is most
likely not what you want.
src/java.base/share/classes/java/util/Locale.java line 333:
> 331: * {@link Locale#US Locale.US} is the {@code Locale} object for the
> United States.</dd>
> 332: * <dt><b>Factory Methods</b></dt>
> 333: * <dd>The method {@link #of(String, String, String)} and its overloads
> obtain a
Same link remarks. Also for `forLanguageTag` below.
src/java.base/share/classes/java/util/Locale.java line 343:
> 341: *
> 342: * {@snippet lang=java :
> 343: * // The following are all equivalent
Suggestion:
* <p>The following are all equivalent:
* {@snippet lang=java :
src/java.base/share/classes/java/util/Locale.java line 376:
> 374: * Locale.Category#FORMAT FORMAT} locale :
> 375: * {@snippet lang=java :
> 376: * // The following are equivalent
Still recommend moving this comment to official javadoc, so the last sentence
can be
The latter uses the default {@link Locale.Category#FORMAT FORMAT} locale, so
the following are equivalent:
src/java.base/share/classes/java/util/Locale.java line 399:
> 397: * resources for multiple locales, it sometimes needs to find one or more
> 398: * locales (or language tags) which meet each user's specific
> preferences. Note
> 399: * that the term "language tag" is used interchangeably with "locale" in
> the following
Should we add an index to "language tag", like:
Suggestion:
* that the term "<dfn>{@index "language tag"}</dfn>" is used interchangeably
with "locale" in the following
src/java.base/share/classes/java/util/Locale.java line 476:
> 474: *
> 475: * <h2>Compatibility</h2>
> 476: * @implNote
I believe `@implNote` is a higher-level tag than a `<h2>`; all following
contents will be indented. Recommended moving the `<h2>` into the `@implNote`.
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.
src/java.base/share/classes/java/util/Locale.java line 2106:
> 2104: * is en_US, getDisplayCountry() will return "France"; if the
> locale is en_US and
> 2105: * inLocale is fr_FR, getDisplayCountry() will return "Etats-Unis".
> 2106: * If the name returned cannot be localized according to inLocale.
Suggestion:
* If the name returned cannot be localized according to inLocale,
-------------
PR Review: https://git.openjdk.org/jdk/pull/22192#pullrequestreview-2443672580
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847236829
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847240545
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847243076
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847253411
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847259756
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847261730
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847263166
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847266745
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847267850
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847270260
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847271943
PR Review Comment: https://git.openjdk.org/jdk/pull/22192#discussion_r1847272982
