On Wed, 10 Apr 2024 20:16:50 GMT, Justin Lu <j...@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
