<i18n dev> RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates

Fri Apr 12 21:00:49 UTC 2024

On Wed, 10 Apr 2024 20:16:50 GMT, Justin Lu <jlu at openjdk.org> wrote:

> Please review this PR which is a large spec reformatting for _java.text.NumberFormat_ and related subclasses, specifically DecimalFormat and CompactNumberFormat.
> 
> The motivation for this change was the difficulty of readability for these classes. This PR consists of changes such as better separating the text into sections with headers, advising to skip the pattern related sections if not needed, and general wording improvements.
> 
> To help with reviewing I have attached a [specdiff](https://cr.openjdk.org/~jlu/JDK-8329222/java.base/java/text/package-summary.html) as well as the built docs: [nFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/NumberFormat.html), [dFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/DecimalFormat.html), and [cnFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/CompactNumberFormat.html);
> 
> I will update the CSR once the wording is finalized.

Hi Justin, I went through the `NumberFormat` part. More to follow.

src/java.base/share/classes/java/text/NumberFormat.java line 61:

> 59:  * {@code NumberFormat} is the abstract base class for all number
> 60:  * formats. This class provides the interface for formatting and parsing
> 61:  * numbers in a {@link Locale localized} manner. This enables code that can be completely

Linking "localized" to the `Locale` class seems a bit odd to me. A user who clicks the link may be stranded.

src/java.base/share/classes/java/text/NumberFormat.java line 64:

> 62:  * independent of the locale conventions for decimal points, thousands-separators,
> 63:  * whether the number format is even decimal, or even the particular decimal
> 64:  * digits used. For example, this class could be used within an application to

I kind of prefer the original wording here, as non-decimal format is the rarest case IMO.

src/java.base/share/classes/java/text/NumberFormat.java line 66:

> 64:  * digits used. For example, this class could be used within an application to
> 65:  * produce a number in a currency format according to conventions of the locale
> 66:  * a user resides in.

"a user resides in" may not convey any additional value. The user doesn't have to be in that locale.

src/java.base/share/classes/java/text/NumberFormat.java line 87:

> 85:  *
> 86:  * Alternatively, if a {@code NumberFormat} for a different locale is required, use
> 87:  * one of the factory method variants that take {@code locale} as a parameter,

Instead of "variants", "overloaded" may be more definitive. Also, "locale" can be capitalized.

src/java.base/share/classes/java/text/NumberFormat.java line 88:

> 86:  * Alternatively, if a {@code NumberFormat} for a different locale is required, use
> 87:  * one of the factory method variants that take {@code locale} as a parameter,
> 88:  * for example, {@link #getIntegerInstance(Locale)}. To determine if the current

"the current locale" -> "a particular locale"

src/java.base/share/classes/java/text/NumberFormat.java line 91:

> 89:  * locale is supported by the installed locale-sensitive service implementation,
> 90:  * either use {@link #getAvailableLocales()} or ensure a factory method call is enclosed
> 91:  * within a try block.

Not sure this is correct. Even if a locale is not "supported" by a locale provider, it won't throw an exception, but fallback to, say ROOT, locale.

src/java.base/share/classes/java/text/NumberFormat.java line 101:

> 99:  * If both "nu" and "rg" are specified, the decimal digits from the "nu"
> 100:  * extension supersedes the implicit one from the "rg" extension. Additionally,
> 101:  * currency formats support the "cf" ({@link #getCurrencyInstance(Locale) Currency

Thanks for adding `cf` extension (I forgot this when I implemented it). Probably it is better to put it in the same level as others (nu/rg), instead of "additionally".

src/java.base/share/classes/java/text/NumberFormat.java line 102:

> 100:  * extension supersedes the implicit one from the "rg" extension. Additionally,
> 101:  * currency formats support the "cf" ({@link #getCurrencyInstance(Locale) Currency
> 102:  * Format style}) extension. Although the LDML specification defines various

It is probably better to use "Unicode extensions", instead of "LDML" which might strand readers.

src/java.base/share/classes/java/text/NumberFormat.java line 106:

> 104:  * Runtime Environment might not support any particular Unicode locale attributes
> 105:  * or key/type pairs.
> 106:  * <p>Below is an example of a "en-US" locale with Thai digits,

"en-US" can be replaced with "US" which implies English and is consistent with other locations.

src/java.base/share/classes/java/text/NumberFormat.java line 118:

> 116:  *
> 117:  * <h2>Customizing NumberFormat</h2>
> 118:  * {@code NumberFormat} provides API to customize formatting and parsing behavior,

Could make this PR a dependent PR of the leniency PR, and add `setStrict()` here as well.

src/java.base/share/classes/java/text/NumberFormat.java line 135:

> 133:  * {@code CompactNumberFormat} depending on the factory method used. For example,
> 134:  * cast to {@code DecimalFormat} to call {@link DecimalFormat#setGroupingSize(int)}
> 135:  * to change the desired digits between grouping separators.

Since there is a possibility that SPIs can return the NumberFormat instances, not sure recommending casting is the right thing. If you want to add this, at least suggest the type check beforehand.

src/java.base/share/classes/java/text/NumberFormat.java line 137:

> 135:  * to change the desired digits between grouping separators.
> 136:  * While this will work for the vast majority of locales; a {@code
> 137:  * try} block should be used in case a non-supported locale is encountered.

As above, try won't catch an exception even with non-supported locales.

src/java.base/share/classes/java/text/NumberFormat.java line 175:

> 173:  *
> 174:  * @implSpec
> 175:  * <h3>Null Parameter Handling</h3>

Not sure we'd want this as a h3 header, as it makes `@implSpec` less significant.

src/java.base/share/classes/java/text/NumberFormat.java line 183:

> 181:  * {@code NullPointerException}.
> 182:  *
> 183:  * <h3>Default RoundingMode</h3>

Same as above.

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

PR Review: https://git.openjdk.org/jdk/pull/18731#pullrequestreview-1998303983
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563111509
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563121307
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563106856
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563137394
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563147282
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563149911
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563164447
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563165884
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563168311
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563180331
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563187344
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563187969
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563192719
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563192836