Skip to content
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

==: Fix to compare with address prefix #69

Closed
wants to merge 1 commit into from

Conversation

taketo1113
Copy link
Contributor

It fixes to compare with an address prefix for == method.
It return false when compared with a different address prefix.

IPAddr.new('10.0.0.0/16') == IPAddr.new('10.0.0.0/8')
=> false # changed behavior

IPAddr.new('10.0.0.0') == IPAddr.new('10.0.0.0')
=> true

IPAddr.new('10.0.0.0/16') == IPAddr.new('10.0.0.0/16')
=> true

Fix #21

@sorah
Copy link
Member

sorah commented Apr 25, 2024

I agree this is a bug but actually this behaviour is long-standing, so I'm slightly afraid of introducing incompatibility

@taketo1113
Copy link
Contributor Author

I agree this is a bug but actually this behaviour is long-standing, so I'm slightly afraid of introducing incompatibility

@sorah Sure, It changes existing behavior.
Do you mean that would need major version up?

I think it needs to fix this bug even if changing behavior, because if not fix this bug for considered incompatibility, it will be a more serious incompatibility.

@knu
Copy link
Member

knu commented Apr 25, 2024

The test case has been there since the very beginning, so I believe this behavior is as intended by the original author Umemoto-san.

I'd like to see a new strict equality method added rather than breaking compatibility.

@knu
Copy link
Member

knu commented Apr 25, 2024

I think we already have eql? that can be used for strict equality test.

@taketo1113 taketo1113 closed this Apr 29, 2024
@taketo1113
Copy link
Contributor Author

@knu Thanks for the comments.
I think too it is better to use eql? method, rather than add another method to compare addresses with prefix.

And I found the same issues in Ruby Issue Tracking System.
I’m going to close this PR.

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

Successfully merging this pull request may close these issues.

== fails
3 participants