toString: Fix bug in emitting exponential notation #69
Conversation
|
There was a problem hiding this comment.
For some reason I get GitHub error messages when trying to comment on individual lines of the PR, and GitHub refuses to save my individual line comments.
My comments:
Why is RenderAmountValueWithFractionDigits doing any rounding? Values stored in Amount are always already rounded to the specified fractionDigits, so rounding here is confusing, unnecessary, and creates opportunities for future bugs. If we ever want to use RenderAmountValueWithFractionDigits for values that are not in Amount, we should add an explicit rounding step.
- Rename numDigits to fractionDigits (or whatever we end up renaming fractionDigits to)
- Delete the roundingMode argument.
- 4.a.ii should use "-0", not "0".
- For clarity move 4.b.i after 4.b.iii. No reason to compute trailingZeroes if we're not going to use it.
- Delete steps 7.b and 8.
- Line 10's language contradicts itself. There is no "unique decimal string representation without duplicate leading zeroes" of, for example ½: ".5", "0.5", and "0.50" are all decimal string representations of ½ without duplicate leading zeroes, and none is unique. Similarly, "03", "3", and "3.0" are all decimal string representations of 3 without duplicate leading zeroes, and none is unique.
- 11 has a missing underscore
- 11.a has a missing underscore
- 12.a has a missing underscore
- I can't make sense of the logic starting with line 11. Line 13 is executed only when e = numDigits, which has the strange effect of exponential notation applying only to numbers with no trailing zeroes:
- 1.21e9 with numDigits set to -7 gets output as "1.21e9", but
- 1.20e9 with numDigits set to -7 gets output as "120000000".
- 1 with no fraction digits (numDigits set to 0) crashes on line 13.d; if we ignore the crash, it gets output as "1e0". That makes no sense.
There was a problem hiding this comment.
We should not be re-defining number serialization. That's already well defined in Number::toString, which should have at least its steps 6-12 extracted into an AO that Amount also uses.
I ran through some examples where we have negative fractional digits and found a couple of bugs in our
toString. In particular, the assertion thatn ≥ 0should have beenn ≤ 0.The main issue was a sign error. Looking at the problematic cases (#54) we would convert, for instance.
"1.23e3"into"1.23e-3"