Improve SSHKit::Host comparison methods #205
Conversation
FooBarWidget
force-pushed
the
equal_fix
branch
from
ec104ed
to
329163b
Compare
* #equal? should test for object identity, not whether the contents are equal, as per the Ruby documentation. * #eql? and #== should check the contents directly instead of comparing the hash. This prevents false positives.
FooBarWidget
force-pushed
the
equal_fix
branch
from
329163b
to
f2b4474
Compare
|
Thanks @FooBarWidget good call. I'll look at your other PR as well today/tomorrow time permitting. Thanks for taking the time to contribute to SSHKit, I respect you Phusion guys massively, and it's a pleasure to receive code from you :) |
|
I'm in the process of closing old/inactive PRs; pardon the noise. This PR seems to have been abandoned. Please comment if you'd like it to be reconsidered. |
|
Hey @mattbrictson, why is this patch considered abandoned? There was no feedback on whether the patch is acceptable or not. So at the very least, the patch was not abandoned on my side. |
|
@FooBarWidget: This was just cleanup due to age. Sorry about that. If you don't mind, could you rebase against master, and then we'll try and get this resolved. Thanks! |
|
Looks like we dropped the ball on this PR, then. Sorry about that! Thanks for sticking around. I will try to review this and make a decision whether to merge by the end of the week. Re-opening. |
There was a problem hiding this comment.
I agree with all the proposed changes, thanks!
However, per the Ruby documentation:
The
hashfunction must have the property thata.eql?(b)impliesa.hash == b.hash.
Right now this is not the case because Host#hash is using port, but Host#eql? is using port_with_default. That means that two hosts could be eql but have different hash values.
Do you agree that the hash implementation should be changed to use port_with_default?
This pull request improves SSHKit::Host comparison in the following ways.