[go: nahoru, domu]

Skip to content
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

Closed
2 tasks done
jparise opened this issue Dec 10, 2022 · 3 comments · Fixed by #693
Closed
2 tasks done
Assignees
Labels
bug Something isn't working

Comments

@jparise
Copy link
Contributor
jparise commented Dec 10, 2022

Is there an existing issue for this?

  • I have searched the existing issues

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 is String, and String(-0) produces '0'.

toDecimal(dinero({ amount: -1, currency: USD }) === '0.01'

Expected behavior

toDecimal(dinero({ amount: -1, currency: USD }) === '-0.01'

Steps to reproduce

Here's a failing test case:

it('returns the negative amount for the last unit in decimal format', () => {
  const d = dinero({ amount: -1, currency: USD });

  expect(toDecimal(d)).toEqual('-0.01');
});
Expected: "-0.01"
Received: "0.01"

Version

2.0.0-alpha12

Environment

Node.js 16.14.1

Code of Conduct

  • I agree to follow this project's Code of Conduct
@jparise jparise added the bug Something isn't working label Dec 10, 2022
@jparise
Copy link
Contributor Author
jparise commented Dec 10, 2022

This wasn't an issue prior to the toUnit / toFormat to toUnits / toDecimal change because the code previous deal with values like -0.01 rather than the decomposed units array [-0, .01].


I spent some time looking at this. Assuming we want to localize the solution to toDecimal and continue to use String as the default toString formatter, it seems like we'd need a special (gross) case like:

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 (Calculator-based?) solution.

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 Object.is identity test doesn't work for Big.js instances and equality checks can't distinguish between 0 and -0 values.

It might be possible for sign to help us out, perhaps by getting it to return -0 for negative zero values and using the Object.is test combined with a calculator.zero() comparison, but that just moves the existing (unsolved) problem into sign.

I'm happy to help explore other solutions.

@sarahdayan
Copy link
Collaborator

Hey @jparise, good catch!

I think it's fine that toUnits returns a positive zero, since it's equivalent in the language and trying to work against it will generate clumsy, unreliable code.

We can fix this in toDecimal by checking two things:

  1. units contains at least one negative value (they should all be negative if the original amount is negative, aside from zeros of course)
  2. The first entry of the units array is zero

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!

@jparise
Copy link
Contributor Author
jparise commented Dec 10, 2022

Thank you! This was the important insight that I was missing:

units contains at least one negative value (they should all be negative if the original amount is negative, aside from zeros of course)

I have a working version based on your suggestions that I'll push shortly. I appreciate the opportunity to finish the puzzle. 😉

jparise added a commit to jparise/dinero.js that referenced this issue Dec 10, 2022
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
jparise added a commit to jparise/dinero.js that referenced this issue Dec 10, 2022
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
jparise added a commit to jparise/dinero.js that referenced this issue Dec 11, 2022
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
jparise added a commit to jparise/dinero.js that referenced this issue Dec 12, 2022
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
sarahdayan added a commit that referenced this issue Dec 19, 2022
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 <5370675+sarahdayan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants