Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
On Tue, 16 Apr 2024 22:30:47 GMT, Justin Lu wrote:
>> src/java.base/share/classes/java/text/CompactNumberFormat.java line 78:
>>
>>> 76: * installed. Thus, to use an instance method defined by {@code
>>> CompactNumberFormat},
>>> 77: * the {@code NumberFormat} returned by the factory method should first
>>> be type
>>> 78: * checked before cast to {@code CompactNumberFormat}. If the installed
>>> locale-sensitive
>>
>> Since `CompactNumberFormat` does not provide its own instance methods (i.e.,
>> instance methods are exactly the same as the parent NumberFormat), "instance
>> methods defined by CompactNF" does not make much sense, which is different
>> for `DecimalFormat`.
>
> `CompactNumberFormat` does define its own instance methods such as
> `setParseBigDecimal`, `setGroupingSize`, etc, so I think that this wording
> should still be included.
My bad, you're correct. The confusion I had was probably this wording: "These
factory methods may not always return a CompactNumberFormat". I believe it
*does* return a CNF (in terms of `instanceof`), because as the factory methods
names imply. However, in the case of DecimalFormat, the factory methods are
simply, for example, "NumberFormat.getInstance()" and the implementation may
return a different class instance, which does not even offer methods on
DecimalFormat class. The difference needs to be described somehow IMO.
>> src/java.base/share/classes/java/text/NumberFormat.java line 235:
>>
>>> 233: * @see ChoiceFormat
>>> 234: * @see CompactNumberFormat
>>> 235: * @see Locale
>>
>> Could be removed as the link is now gone
>
> As this is a localized class, and the class description mentions `Locale` in
> various places, I feel like `Locale` should still be included as a `see` tag,
> (even though I removed the locale link). I am fine either way, what do you
> think?
OK, let's keep them then
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568018900
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568020362
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
On Tue, 16 Apr 2024 21:14:28 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains five commits:
>>
>> - merge master and add setStrict() to nFmt class spec
>> - implement suggestions from dFmt review
>> - implement suggestions from cnFmt review
>> - implement suggestions from nFmt review
>> - init
>
> src/java.base/share/classes/java/text/CompactNumberFormat.java line 78:
>
>> 76: * installed. Thus, to use an instance method defined by {@code
>> CompactNumberFormat},
>> 77: * the {@code NumberFormat} returned by the factory method should first
>> be type
>> 78: * checked before cast to {@code CompactNumberFormat}. If the installed
>> locale-sensitive
>
> Since `CompactNumberFormat` does not provide its own instance methods (i.e.,
> instance methods are exactly the same as the parent NumberFormat), "instance
> methods defined by CompactNF" does not make much sense, which is different
> for `DecimalFormat`.
`CompactNumberFormat` does define its own instance methods such as
`setParseBigDecimal`, `setGroupingSize`, etc, so I think that this wording
should still be included.
> src/java.base/share/classes/java/text/NumberFormat.java line 91:
>
>> 89: * for example, {@link #getIntegerInstance(Locale)}. If the installed
>> locale-sensitive
>> 90: * service implementation does not support the given {@code Locale},
>> {@link Locale#ROOT}
>> 91: * will be used as the fallback {@code Locale}.
>
> The fallback traverses the parent locales, and checks if it is supported or
> not. So it may or may not reach Locale.ROOT. How about "it looks up the
> parent locale chain and uses the one that is supported" or something along
> the lines?
Thanks for the clarification, updated the wording.
> src/java.base/share/classes/java/text/NumberFormat.java line 235:
>
>> 233: * @see ChoiceFormat
>> 234: * @see CompactNumberFormat
>> 235: * @see Locale
>
> Could be removed as the link is now gone
As this is a localized class, and the class description mentions `Locale` in
various places, I feel like `Locale` should still be included as a `see` tag,
(even though I removed the locale link). I am fine either way, what do you
think?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568004434
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568004370
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568004906
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
On Tue, 16 Apr 2024 17:06:50 GMT, Justin Lu wrote:
>> Justin Lu has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains five commits:
>>
>> - merge master and add setStrict() to nFmt class spec
>> - implement suggestions from dFmt review
>> - implement suggestions from cnFmt review
>> - implement suggestions from nFmt review
>> - init
>
> src/java.base/share/classes/java/text/NumberFormat.java line 115:
>
>> 113: * attributes or key/type pairs.
>> 114: * Below is an example of a "US" locale currency format with
>> accounting style,
>> 115: * {@code
>> NumberFormat.getInstance(Locale.forLanguageTag("en-US-u-cf-account"));}
>
> I replaced the previous Thai digits example with an account currency format
> style, as it is probably a more common use case.
Good. I think adding the expectation would be more helpful, such as,
"accounting style, that is, a negative value is enclosed in parentheses instead
of a prepended minus sign."
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567934872
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
On Tue, 16 Apr 2024 17:12:06 GMT, Justin Lu 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.
>
> Justin Lu has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains five commits:
>
> - merge master and add setStrict() to nFmt class spec
> - implement suggestions from dFmt review
> - implement suggestions from cnFmt review
> - implement suggestions from nFmt review
> - init
Thanks for the changes. Looks much better.
src/java.base/share/classes/java/text/CompactNumberFormat.java line 78:
> 76: * installed. Thus, to use an instance method defined by {@code
> CompactNumberFormat},
> 77: * the {@code NumberFormat} returned by the factory method should first
> be type
> 78: * checked before cast to {@code CompactNumberFormat}. If the installed
> locale-sensitive
Since `CompactNumberFormat` does not provide its own instance methods (i.e.,
instance methods are exactly the same as the parent NumberFormat), "instance
methods defined by CompactNF" does not make much sense, which is different for
`DecimalFormat`.
src/java.base/share/classes/java/text/CompactNumberFormat.java line 80:
> 78: * checked before cast to {@code CompactNumberFormat}. If the installed
> locale-sensitive
> 79: * service implementation does not support the given locale, {@link
> Locale#ROOT}
> 80: * will be used as a fallback.
Same with `NumberFormat`.
src/java.base/share/classes/java/text/DecimalFormat.java line 88:
> 86: * NumberFormat nFmt = NumberFormat.getCurrencyInstance(Locale.US);
> 87: * if (nFmt instanceof DecimalFormat dFmt) {
> 88: * // cast to DecimalFormat to use setPositiveSuffix(String)
This comment seems incorrect. No casting is made here.
src/java.base/share/classes/java/text/DecimalFormat.java line 421:
> 419: * @see DecimalFormatSymbols
> 420: * @see ParsePosition
> 421: * @see Locale
Can be removed
src/java.base/share/classes/java/text/NumberFormat.java line 91:
> 89: * for example, {@link #getIntegerInstance(Locale)}. If the installed
> locale-sensitive
> 90: * service implementation does not support the given {@code Locale},
> {@link Locale#ROOT}
> 91: * will be used as the fallback {@code Locale}.
The fallback traverses the parent locales, and checks if it is supported or
not. So it may or may not reach Locale.ROOT. How about "it looks up the parent
locale chain and uses the one that is supported" or something along the lines?
src/java.base/share/classes/java/text/NumberFormat.java line 235:
> 233: * @see ChoiceFormat
> 234: * @see CompactNumberFormat
> 235: * @see Locale
Could be removed as the link is now gone
-
PR Review: https://git.openjdk.org/jdk/pull/18731#pullrequestreview-2004548849
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567948678
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567949084
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567963269
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567966048
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567910502
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567937751
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
On Mon, 15 Apr 2024 18:43:29 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains five commits:
>>
>> - merge master and add setStrict() to nFmt class spec
>> - implement suggestions from dFmt review
>> - implement suggestions from cnFmt review
>> - implement suggestions from nFmt review
>> - init
>
> Here's the last part of the comments for `DecimalFormat`
@naotoj The latest commits should implement your batch of suggestions. I have
also re-built the docs and specdiff with these changes.
> src/java.base/share/classes/java/text/CompactNumberFormat.java line 154:
>
>> 152: * ) 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)
I originally removed it because the syntax already covered proper usage. But I
can see how it can be confusing at first to understand. I re-inserted the
original wording, but modified it IMO to be easier to understand.
> 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?
I moved it before `Parsing` as it is related to formatting, so its better under
the `Formatting` section.
All the other CompactNumberFormat comments should be implemented as well.
> 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.
OK, that's a good point. In all 3 of these classes, I adjusted the wording to
recommend using the factory methods for a locale's standard formatting.
However, I made it apparent that the resultant Object can be any subclass of
NumberFormat, and that if users want to cast, (so that they have access to the
subclass instance methods), they should do appropriate type checking first.
All the other DecimalFormat comments you left should be implemented as well.
> src/java.base/share/classes/java/text/NumberFormat.java line 118:
>
>> 116: *
>> 117: * Customizing NumberFormat
>> 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.
The leniency PR was integrated, I directly added setStrict() to this PR
-
PR Comment: https://git.openjdk.org/jdk/pull/18731#issuecomment-2059552391
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567700054
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567696852
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567696727
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567698329
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
On Tue, 16 Apr 2024 17:09:04 GMT, Justin Lu 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.
>
> Justin Lu has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains five commits:
>
> - merge master and add setStrict() to nFmt class spec
> - implement suggestions from dFmt review
> - implement suggestions from cnFmt review
> - implement suggestions from nFmt review
> - init
src/java.base/share/classes/java/text/NumberFormat.java line 115:
> 113: * attributes or key/type pairs.
> 114: * Below is an example of a "US" locale currency format with
> accounting style,
> 115: * {@code
> NumberFormat.getInstance(Locale.forLanguageTag("en-US-u-cf-account"));}
I replaced the previous Thai digits example with an account currency format
style, as it is probably a more common use case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567697780
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
> 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. Justin Lu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - merge master and add setStrict() to nFmt class spec - implement suggestions from dFmt review - implement suggestions from cnFmt review - implement suggestions from nFmt review - init - Changes: https://git.openjdk.org/jdk/pull/18731/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18731&range=01 Stats: 597 lines in 3 files changed: 233 ins; 204 del; 160 mod Patch: https://git.openjdk.org/jdk/pull/18731.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18731/head:pull/18731 PR: https://git.openjdk.org/jdk/pull/18731
