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. Comments for the `CompactNumberFormat`. src/java.base/share/classes/java/text/CompactNumberFormat.java line 50: > 48: * <p> > 49: * {@code CompactNumberFormat} is a concrete subclass of {@code > NumberFormat} > 50: * that formats a decimal number in a {@link Locale localized} compact > form. Same as in the NumberFormat src/java.base/share/classes/java/text/CompactNumberFormat.java line 63: > 61: * <ul> > 62: * <li> Use {@link NumberFormat#getCompactNumberInstance()} > 63: * to obtain a {@code CompactNumberFormat} for the default locale. Might add `Style.SHORT` as the default too src/java.base/share/classes/java/text/CompactNumberFormat.java line 74: > 72: * <small>Note: It is recommended to use one of the NumberFormat factory > methods > 73: * which is tailored to the conventions of the given locale to retrieve a > 74: * CompactNumberFormat.</small> Not sure using the small font would make any significance here. Also, I'd make this a conditional sentence, i.e, if the user wants the standard formatting for a locale and a style, to avoid blind recommendation. src/java.base/share/classes/java/text/CompactNumberFormat.java line 98: > 96: * The {@code compactPatterns} in {@link > 97: * CompactNumberFormat#CompactNumberFormat(String, DecimalFormatSymbols, > String[]) > 98: * CompactNumberFormat(decimalPattern, symbols, compactPatterns}} are > represented Nit: `}` -> `)` src/java.base/share/classes/java/text/CompactNumberFormat.java line 103: > 101: * > 102: * <p><b>For those planning to only use the factory methods, the pattern > syntax may > 103: * not be relevant. If this is the case, continue reading at the {@link > ##formatting Instead of adding reading suggestions like this in bold text, how about moving this `Compact Number Patterns` section to the very end of the class description, as it is less important for normal usage? src/java.base/share/classes/java/text/CompactNumberFormat.java line 129: > 127: * minimum integer digits, and suffix. The negative subpattern > 128: * is optional, if absent, then the positive subpattern prefixed with the > 129: * minus sign ({@code '-'}) is used as the negative The code point and its name should be preserved. "minus sign" can be ambiguous, as "MINUS SIGN" code point is actually `U+2212` src/java.base/share/classes/java/text/CompactNumberFormat.java line 142: > 140: * {@linkplain DecimalFormat##special_pattern_character Special > characters}, > 141: * on the other hand, stand for other characters, strings, or classes of > 142: * characters. These characters must be quoted using single quotes > ({@code '}) Same as above src/java.base/share/classes/java/text/CompactNumberFormat.java line 154: > 152: * </sup>) in the US locale can be specified as the SimplePattern: > {@code "0 Million"}, for the > 153: * German locale it can be specified as the PluralPattern: > 154: * {@code "{one:0' 'Million other:0' 'Millionen}"}. I like the German example, however, I think the original explanation for the pattern should be preserved (wording might be better though) src/java.base/share/classes/java/text/CompactNumberFormat.java line 207: > 205: * {@link java.math.RoundingMode} for formatting. By default, it uses > 206: * {@link java.math.RoundingMode#HALF_EVEN RoundingMode.HALF_EVEN}. > 207: * Any reason for this part to move before the `Parsing` section? ------------- PR Review: https://git.openjdk.org/jdk/pull/18731#pullrequestreview-1998701154 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563335521 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563346260 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563343344 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563354139 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563353485 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563355976 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563356745 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1566208653 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1566221378
