fix(up, down, halfUp): fix handling of numbers close to 0 and rounding of integer results

[shasderias]

up/down - Functions should not round integer results.

up - up should only round positive numbers, 0 is not a positive number.

halfUp - halfUp was incorrectly rounding numbers in the [0,1) range due to the incorrect definition of isPositive.

Fixes #710
Fixes #713

Written by throwing tests at the wall and making them pass, would appreciate if someone could review their correctness.

In the rounding functions, is it necessary to handle the case where factor is negative? If so, the definition of isPositive in some of the functions may not handle this case.

From a big picture going to beta POV, some of these rounding functions are really obscure (e.g. halfOdd) and have little practical application. They also feel non-trivial to get right, do we really need all of them?

Also, rounding to -0 is correct but surprising, should something be done here?

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
dinerojs	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 19, 2023 at 4:20PM (UTC)

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5b05403:

Sandbox	Source
@dinero.js/example-cart-react	Configuration
@dinero.js/example-cart-vue	Configuration
@dinero.js/example-pricing-react	Configuration
@dinero.js/example-starter	Configuration
laughing-fog-nsqxfk	Issue #710
gracious-thunder-jlghkd	Issue #713

[johnhooks]

@shasderias this PR fixes #713 too. I added failing tests to confirm the fix. I'll add a review of this PR too.

[johnhooks]

This is awesome work, you were very thorough.

I'm reading up on half even and half odd rounding, of the new tests, I had a little trouble wrapping my head around if they are being properly tested.

[johnhooks]

Why are these flipped? All the tests pass if I leave them as they were.

[shasderias]

To be consistent with preceding comment explaining what I'm doing.

I also reckon humans handle positive numbers better, so this can be viewed as: (1) addressing the problem, then (2) the common case then (3) the less common case.

I don't feel strongly about this, except that the comment should be amended if this change is reverted.

[johnhooks]

That makes sense to me, I was just checking that there was a reason. It wasn't apparent.

[shasderias]

This is awesome work, you were very thorough.

I'm reading up on half even and half odd rounding, of the new tests, I had a little trouble wrapping my head around if they are being properly tested.

My internal logic is:
(1) if the last digit is not 5, we're not dealing with half, round to closest integer
(2) 0 is even, 1 is odd, -1 is also odd

[johnhooks]

@shasderias I understand it better now, when I took a look at the edits in context of the existing halfOdd and halfEven tests. The existing tests already checked for rounding toward odd or even, the tests you added were to make sure it handles rounding a single base unit in the expected way.

[sarahdayan]

Fantastic work @shasderias, thanks for the contirbution! I took the liberty to add a few commits instead of making suggestions, to avoid unnecessary back-and-forths. My changes are mostly refactors:

• Move your comment to JSDoc, which has the nice sife-effect of documenting the rounding functions.
• Use property tests instead of fixed test cases for when we want to work with arbitrary values that follow certain conditions.