Skip to content

Conversation

@jimpo
Copy link

@jimpo jimpo commented Jun 17, 2015

#470 made a breaking change to the behavior of Money#==. This reverts the change in #470 while still removing the deprecation warning.

Ruby 2.2 warns that <=> should return nil on invalid comparisons rather than raise an error.

Fixes #469.

@semmons99
Copy link
Member

Waiting for all my green lights, then I'll merge.

semmons99 added a commit that referenced this pull request Jun 17, 2015
Fix Money#== and Ruby 2.2 deprecation warning
@semmons99 semmons99 merged commit 58ebf1f into RubyMoney:master Jun 17, 2015
@printercu
Copy link
Member

@jimpo @semmons99 there are warning after this pr was merged:

~/.rvm/gems/ruby-2.2.1/gems/rspec-expectations-3.2.1/lib/rspec/matchers/built_in/eq.rb:35: warning: Return nil in #<=> if the comparison is inappropriate or avoid such comparison.

@jimpo
Copy link
Author

jimpo commented Jun 18, 2015

Interesting. Is that in the test suite for this gem or another application? Could you show the line in your application that caused it?

@printercu
Copy link
Member

i've run rspec in gem

@semmons99
Copy link
Member

Yeah. Please provide more info as we're not able to reproduce locally or on CI.

@jimpo
Copy link
Author

jimpo commented Jun 18, 2015

It issues the warning in Ruby 2.2.1, not Ruby 2.2.2. The issue is that Money#<=> is throwing a Money::Bank::UnknownRate error when comparing two non-zero amounts with different currencies in the test suite.

I guess we need to decide what the behavior of Money#== should be in this case. money1.eql?(money2) is false because the currencies are not the same, but if an exchange rate is defined and the converted amounts are equal, money1 <=> money2 == 0. money1 == money2 could either always be false if the currencies are different and the amounts are nonzero, or it could possibly raise a Money::Bank::UnknownRate error. Thoughts?

@semmons99
Copy link
Member

Can you open another ticket and ping all collabs? Also, feel free to submit a PR that just does what you think it should 😄

@jimpo
Copy link
Author

jimpo commented Jun 18, 2015

Continuing discussion here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants