fix(Update toRoundedUnit to properly round negative values) #10
Conversation
There was a problem hiding this comment.
Thanks for this! There is very little to edit in the PR itself.
There's an issue with the commit message though. I think you got mixed up with scope and commit message. Your commit message should look more like: fix(dinero#toRoundedUnit): Update toRoundedUnit to properly round negative values
There also are typos in the long form message.
I think you can manually change the commit message but if not this may need to be a new PR 😕.
src/dinero.js
Outdated
| const roundedUnit = | ||
| sign * Math.round(Math.abs(calculator.multiply(this.toUnit(), factor))) | ||
|
|
||
| return calculator.divide(roundedUnit, factor) |
There was a problem hiding this comment.
Since sign and roundedUnit are only used once, they could be avoided altogether.
return calculator.divide(
Math.sign(this.toUnit()) * Math.round(Math.abs(calculator.multiply(this.toUnit(), factor))),
factor
)
tarnelope
force-pushed
the
negative-value-rounding
branch
from
0669e66
to
28c92cf
Compare
|
Sorry about that! I noticed those afterwards and wasn't sure if I could edit the message but I think I figured it out. |
src/dinero.js
Outdated
| return calculator.divide( | ||
| Math.round(calculator.multiply(this.toUnit(), factor)), | ||
| Math.sign(this.toUnit()) * |
There was a problem hiding this comment.
I was just thinking this should be done with Calculator#multiply as well. Will be more future-proof when support for big integers is added.
calculator.multiply(Math.sign(this.toUnit()), Math.round(Math.abs(calculator.multiply(this.toUnit(), factor))))
…ative values Math.round() intrinsically does not correctly round negative values so this change first rounds the absolute value of the given amount and then multiplies in the Math.sign() afterwards. fix dinerojs#7
tarnelope
force-pushed
the
negative-value-rounding
branch
from
28c92cf
to
508cca3
Compare
|
🎉 This PR is included in version 1.0.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Math.round() doesn't correctly round negative values so this change first rounds the absolute value of the given amount and then multiplies in the Math.sign() afterwards.
This is my first contribution so happy for any feedback and change requests.
fix #7