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. Here's the last part of the comments for `DecimalFormat` src/java.base/share/classes/java/text/DecimalFormat.java line 58: > 56: /** > 57: * {@code DecimalFormat} is a concrete subclass of > 58: * {@code NumberFormat} that formats decimal numbers in a {@link Locale > localized} manner. Same as the other two classes. src/java.base/share/classes/java/text/DecimalFormat.java line 93: > 91: * for example, {@link #getIntegerInstance(Locale)}. {@link > 92: * NumberFormat#getAvailableLocales()} can be used to determine if the > desired > 93: * locale is supported. I don't think this section is helpful, or rather, may mislead the users. As the original text explains, factory methods in `NumberFormat` do not guarantee to return `DecimalFormat` instances by design. src/java.base/share/classes/java/text/DecimalFormat.java line 116: > 114: * <p><b>For those planning to only use the factory methods, the pattern > syntax may > 115: * not be relevant. If this is the case, continue reading at the {@link > ##formatting Formatting and Parsing} > 116: * section.</b> As in the `CompactNumberFormat`, I suggest moving the `DecimalFormat Pattern` section at the end. src/java.base/share/classes/java/text/DecimalFormat.java line 168: > 166: * localized symbol. > 167: * When {@link #applyLocalizedPattern(String)} is called, the default > symbols lose their > 168: * syntactical meaning, and vice versa with {@link > #applyPattern(String)} with exception Retain the original wording that explains localized and non-localized patterns at the beginning. Using the method example for the localized patterns might be a bit jump in the context. src/java.base/share/classes/java/text/DecimalFormat.java line 255: > 253: * integer digits will be derived from the pattern. This derivation is > detailed > 254: * in the {@link ##scientific_notation Scientific Notation} section. > This behavior > 255: * is the typical end-user desire; {@link #setMaximumIntegerDigits(int)} > can be "This behavior ... desire" can be removed. src/java.base/share/classes/java/text/DecimalFormat.java line 263: > 261: * subpattern has a prefix, numeric part, and suffix. The negative > subpattern > 262: * is optional; if absent, then the positive subpattern prefixed with the > 263: * minus sign ({@code '-'}) is used as the Keep the code point/name for hyphen-minus src/java.base/share/classes/java/text/DecimalFormat.java line 398: > 396: * <h3>Special Values</h3> > 397: * <ul> > 398: * <li><p><b>Not a Number</b> ({@code NaN}) is successfully formatted as > a string, I'd not add `successfully` here. ------------- PR Review: https://git.openjdk.org/jdk/pull/18731#pullrequestreview-2001855368 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1566248166 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1566260898 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1566263120 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1566272248 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1566279017 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1566280153 PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1566266870
