-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug]: toDecimal fails to print a leading negative sign when the first unit is negative zero #692
Comments
This wasn't an issue prior to the I spent some time looking at this. Assuming we want to localize the solution to if (isFirst && Object.is(unit, -0) /* && !unitAsPaddedString.startsWith('-') */) {
return `-${unitAsString}`;
} That works for native JavaScript numbers, but it won't work for bigint or Big.js amounts. We'd need a more generic ( I tried constructing a generic "negative zero" reference value: const zero = calculator.zero();
const negativeOne = calculator.decrement(zero);
const negativeZero = calculator.multiply(zero, negativeOne); ... but that doesn't work because (a) the It might be possible for I'm happy to help explore other solutions. |
Hey @jparise, good catch! I think it's fine that We can fix this in
If both conditions are satisfied, we can pad the returned value with I have a working prototype, but if you want to contribute, I'll let you take a stab at it! |
Thank you! This was the important insight that I was missing:
I have a working version based on your suggestions that I'll push shortly. I appreciate the opportunity to finish the puzzle. 😉 |
Change dinerojs#690 fixed the general case of formatting negative unit values, but was one remaining case where formatting was still incorrect: when the first unit is -0, the resulting string didn't include the leading negative sign. This change identifies that exact case (negative value, leading zero) and pads the resulting string with a leading negative sign. Fixes dinerojs#692
Change dinerojs#690 fixed the general case of formatting negative unit values, but was one remaining case where formatting was still incorrect: when the first unit is -0, the resulting string didn't include the leading negative sign. This change identifies that exact case (negative value, leading zero) and pads the resulting string with a leading negative sign. Fixes dinerojs#692
Change dinerojs#690 fixed the general case of formatting negative unit values, but was one remaining case where formatting was still incorrect: when the first unit is -0, the resulting string didn't include the leading negative sign. This change identifies that exact case (negative value, leading zero) and pads the resulting string with a leading negative sign. Fixes dinerojs#692
Change dinerojs#690 fixed the general case of formatting negative unit values, but was one remaining case where formatting was still incorrect: when the first unit is -0, the resulting string didn't include the leading negative sign. This change identifies that exact case (negative value, leading zero) and pads the resulting string with a leading negative sign. Fixes dinerojs#692
Change #690 fixed the general case of formatting negative unit values, but was one remaining case where formatting was still incorrect: when the first unit is -0, the resulting string didn't include the leading negative sign. This change identifies that exact case (negative value, leading zero) and pads the resulting string with a leading negative sign. Fixes #692 Co-authored-by: Sarah Dayan <[email protected]>
Is there an existing issue for this?
Current behavior
#690 fixed the the general case of formatting negative unit values, but there is one outstanding specific case where formatting is still incorrect: when the first unit is -0, the resulting string doesn't include the leading negative sign.
This occurs because the (default)
toString
formatter isString
, andString(-0)
produces'0'
.Expected behavior
Steps to reproduce
Here's a failing test case:
Version
2.0.0-alpha12
Environment
Node.js 16.14.1
Code of Conduct
The text was updated successfully, but these errors were encountered: