Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented [v2]
On Mon, 22 May 2023 23:31:37 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review: Use Static dfs, error message, and exponent digit. Hard code min
>> and max digits instead of calculating at runtime
>
> test/jdk/java/text/Format/DecimalFormat/MantissaDigits.java line 64:
>
>> 62: for (double number : numbers) {
>> 63: // Count the significant digits in the pre-formatted number
>> 64: int originalNumDigits = String.valueOf(number)
>
> It's OK as it stands, but you could write:
>
> str.chars().filter(Character::isDigit).count()
>
> which might be more readable
Like this much better, incorporated!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14066#discussion_r1201538926
Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented [v2]
On Mon, 22 May 2023 23:03:10 GMT, Justin Lu wrote:
>> Please review this PR which updates the Scientific Notation section of
>> Decimal Format. It aims to resolve
>> [JDK-8159023](https://bugs.openjdk.org/browse/JDK-8159023) as well as
>> [JDK-6282188](https://bugs.openjdk.org/browse/JDK-6282188).
>>
>> **Scientific Notation** in Decimal Format contains the definition for a
>> scientific notation formatted number's mantissa as such: _The number of
>> significant digits in the mantissa is the sum of the minimum integer and
>> maximum fraction digits, and is unaffected by the maximum integer digits.
>> For example, 12345 formatted with "##0.##E0" is "12.3E3"._
>>
>> Both the definition and example are incorrect, as the actual result is
>> "12.E345".
>>
>> The following example data show that scientific notation formatted numbers
>> do not adhere to that definition. As, according to the definition, the
>> following numbers should have 3 significant digits, but in reality, they
>> have up to 5.
>>
>> 123 formatted by ##0.#E0 is 123E0
>> 1234 formatted by ##0.#E0 is 1.234E3
>> 12345 formatted by ##0.#E0 is 12.34E3
>> 123456 formatted by ##0.#E0 is 123.5E3
>> 1234567 formatted by ##0.#E0 is 1.235E6
>> 12345678 formatted by ##0.#E0 is 12.35E6
>> 123456789 formatted by ##0.#E0 is 123.5E6
>>
>>
>> The actual definition of the mantissa, as well as examples and further
>> context are included in this change. In addition, a test has been added that
>> tests various patterns to numbers and ensures the format follows the new
>> definition's mathematical expression.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review: Use Static dfs, error message, and exponent digit. Hard code min
> and max digits instead of calculating at runtime
Thanks, Justin. Left some super-nits.
test/jdk/java/text/Format/DecimalFormat/MantissaDigits.java line 52:
> 50: 1.1234, 1., 1.412, 222.333, -771.
> 51: };
> 52: private static final DecimalFormatSymbols dfs = new
> DecimalFormatSymbols(Locale.US);
The `static final` field is better uppercased.
test/jdk/java/text/Format/DecimalFormat/MantissaDigits.java line 64:
> 62: for (double number : numbers) {
> 63: // Count the significant digits in the pre-formatted number
> 64: int originalNumDigits = String.valueOf(number)
It's OK as it stands, but you could write:
str.chars().filter(Character::isDigit).count()
which might be more readable
-
PR Review: https://git.openjdk.org/jdk/pull/14066#pullrequestreview-1437867369
PR Review Comment: https://git.openjdk.org/jdk/pull/14066#discussion_r1201295121
PR Review Comment: https://git.openjdk.org/jdk/pull/14066#discussion_r1201295601
Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented [v2]
On Mon, 22 May 2023 20:41:29 GMT, Naoto Sato wrote: >> Justin Lu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Review: Use Static dfs, error message, and exponent digit. Hard code min >> and max digits instead of calculating at runtime > > test/jdk/java/text/Format/DecimalFormat/MantissaDigits.java line 89: > >> 87: // Test the new definition of the Mantissa >> 88: Integer calculatedDigits = Math >> 89: .min(Math.max(zeroes, originalNumDigits), >> (hashes+zeroes)); > > Could we make those expected numbers into the `Arguments` so that there is no > need to calculate in the test case at runtime? I think that would be more > readable. Thanks for the review, addressed both suggestions in the most recent commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/14066#discussion_r1201272651
Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented [v2]
> Please review this PR which updates the Scientific Notation section of > Decimal Format. It aims to resolve > [JDK-8159023](https://bugs.openjdk.org/browse/JDK-8159023) as well as > [JDK-6282188](https://bugs.openjdk.org/browse/JDK-6282188). > > **Scientific Notation** in Decimal Format contains the definition for a > scientific notation formatted number's mantissa as such: _The number of > significant digits in the mantissa is the sum of the minimum integer and > maximum fraction digits, and is unaffected by the maximum integer digits. For > example, 12345 formatted with "##0.##E0" is "12.3E3"._ > > Both the definition and example are incorrect, as the actual result is > "12.E345". > > The following example data show that scientific notation formatted numbers do > not adhere to that definition. As, according to the definition, the following > numbers should have 3 significant digits, but in reality, they have up to 5. > > 123 formatted by ##0.#E0 is 123E0 > 1234 formatted by ##0.#E0 is 1.234E3 > 12345 formatted by ##0.#E0 is 12.34E3 > 123456 formatted by ##0.#E0 is 123.5E3 > 1234567 formatted by ##0.#E0 is 1.235E6 > 12345678 formatted by ##0.#E0 is 12.35E6 > 123456789 formatted by ##0.#E0 is 123.5E6 > > > The actual definition of the mantissa, as well as examples and further > context are included in this change. In addition, a test has been added that > tests various patterns to numbers and ensures the format follows the new > definition's mathematical expression. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Review: Use Static dfs, error message, and exponent digit. Hard code min and max digits instead of calculating at runtime - Changes: - all: https://git.openjdk.org/jdk/pull/14066/files - new: https://git.openjdk.org/jdk/pull/14066/files/e6cbbeea..faa32ec6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14066=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14066=00-01 Stats: 35 lines in 1 file changed: 6 ins; 12 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/14066.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14066/head:pull/14066 PR: https://git.openjdk.org/jdk/pull/14066
