Skip to content

Conversation

@smortex
Copy link
Contributor

@smortex smortex commented Mar 16, 2016

Comparing IPAddress::IPv4 or IPAddress::IPv6 instances against another class currently raises an exception… Returning nil is more appropriate since "42" <=> 42 #=> nil.

@francisluong
Copy link
Contributor

Raising has the benefit that we know that an invalid comparison between unlike types has occurred.

If we return nil, then we would not know that we fat-fingered in a situation like this:

ip1 = IPAddress('192.168.1.1')
ip2 = '192.168.1.1'
if ip1.eql?(ip2)
   # blah
end

@smortex
Copy link
Contributor Author

smortex commented Mar 19, 2016

Your code snippet does not currently raise an exception :-) it returns false because:

  • eql? is not aliased to the overriden ==, so you are in fact calling Object#eql? (see Make eql? behare like ==. #77);
  • Even if it was, the Comparable module currently catch the exception raised in <=> (this will change in the future, this is the reason of this PR).

My point is that with Ruby 2.2.0 (at least), the Comparable module relies on the <=> operator and expects it to not raise exceptions. Here is the warning issued in irb(1):

irb(main):001:0> RUBY_VERSION
=> "2.2.0"
irb(main):002:0> require 'ipaddress'
/var/www/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/ipaddress-0.8.3/lib/ipaddress.rb:72: warning: mismatched indentations at 'end' with 'unless' at 70
/var/www/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/ipaddress-0.8.3/lib/ipaddress/prefix.rb:81: warning: mismatched indentations at 'end' with 'class' at 21
/var/www/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/ipaddress-0.8.3/lib/ipaddress/ipv4.rb:647: warning: method redefined; discarding old multicast?
/var/www/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/ipaddress-0.8.3/lib/ipaddress/ipv4.rb:633: warning: previous definition of multicast? was here
=> true
irb(main):003:0> [IPAddress.parse('::1')].include? IPAddress.parse('127.0.0.1')
(irb):3: warning: Comparable#== will no more rescue exceptions of #<=> in the next release.
(irb):3: warning: Return nil in #<=> if the comparison is inappropriate or avoid such comparison.
=> false

(The first bunch of issues (require) is fixed by applying #59 and #74. This bug reports is related to the two last lines warnings regarding <=>.)

Because the behavior will change in the future, it should make sense to have the IPAddress objects behave like other Ruby objects when compared to unrelated objects, that is returning nil and not raising exceptions. This allows to have the same behavior when reversing the comparison that currently is quite confusing:

irb(main):004:0> IPAddress.parse('::1') <=> 42
NoMethodError: undefined method `to_u128' for 42:Fixnum
    from /var/www/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/ipaddress-0.8.3/lib/ipaddress/ipv6.rb:490:in `<=>'
    from (irb):4
    from /var/www/.rbenv/versions/2.2.0/bin/irb:11:in `<main>'
irb(main):005:0> 42 <=> IPAddress.parse('::1')
=> nil

@aaronchi
Copy link

related: RubyMoney/money#528

@asomers
Copy link
Contributor

asomers commented Aug 29, 2017

Returning nil is correct. See the <=> operator's definition in the Object class
https://ruby-doc.org/core-2.4.1/Object.html#method-i-3C-3D-3E

@smortex
Copy link
Contributor Author

smortex commented Aug 30, 2017

@francisluong, I have removed my branch quite some time ago and can't reconnect this work to the PR. I created another branch for testing purpose and the test suite passes.

This should unblock some issues 😉

Copy link
Contributor

@francisluong francisluong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved based on previous tests. Also... confirmed "If the other object is not comparable then the <=> operator should return nil."

@francisluong francisluong merged commit 9dedeaf into ipaddress-gem:master Aug 30, 2017
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.

4 participants