fix(up, down, halfUp): fix handling of numbers close to 0 and rounding of integer results #711
Conversation
…g of integer results 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 dinerojs#710
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
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:
|
|
@shasderias this PR fixes #713 too. I added failing tests to confirm the fix. I'll add a review of this PR too. |
| (isPositive && !isLessThanHalf) || | ||
| (!isPositive && isLessThanHalf) |
There was a problem hiding this comment.
Why are these flipped? All the tests pass if I leave them as they were.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That makes sense to me, I was just checking that there was a reason. It wasn't apparent.
My internal logic is: |
|
@shasderias I understand it better now, when I took a look at the edits in context of the existing |
|
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:
|
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?