-
Notifications
You must be signed in to change notification settings - Fork 47
correctly handle Float::INFINITY #474
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
Conversation
|
|
||
| def <=>(other) | ||
| return unless other.respond_to?(:to_money) | ||
| if other.is_a?(Numeric) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will also speed up significantly comparing with a Numeric by not casting these to money objects
08363a5 to
1dac65f
Compare
|
what's the plan for merging this, once it's merged the existing code that uses infinity will automatically fail in production? |
|
nothing should be doing |
ryanquanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All LGTM!
|
I understand that it will be caught during local development. However if there is already infinity being used in production, does this change breaks those functionalities? |
|
@JingxianHou yes this is a breaking change for people using |
|
moved raising to #476 |
closes #276
Why
Infinity was never explicitly supported by the money gem, yet was not raising, creating issues like #276
Also some code tries to compare a money object with Infinity, which results in exceptions for currencies with no decimals like JPY, ex:
What
Before
After