IPv4Address and UNIXAddress not-equal comparison is broken

[ivank]

Both IPv4Address and UNIXAddress implement __eq__, but not __ne__.

>>> from twisted.internet.address import IPv4Address
>>> IPv4Address('TCP', '127.0.0.1', 10000) == IPv4Address('TCP', '127.0.0.1', 10000) 
True
>>> IPv4Address('TCP', '127.0.0.1', 10000) != IPv4Address('TCP', '127.0.0.1', 10000) 
True

[exarkun]

Using twisted.python.util.FancyEqMixin should make this a snap to fix.

[awclin]

The test returned a failure ('None' result instead of false) prior to the patch, and passed after. I believe I'm testing the explicit problem detailed in the ticket, though not sure if its advisable to increase the coverage in this area as well?

Post patch, i noticed a test failing in twisted/test/test_udp.py which i've updated to reflect the changes to the equality operator.

[exarkun]

(In [30566]) Branching to 'address-comparison-4817'

[exarkun]

(In [30567]) Apply 4817-1.patch

refs #4817

[exarkun]

Thanks!

1. Since testOldAddress no longer emits a warning, the suppression for it should be deleted. Also, can you add a docstring and rename it to test_oldAddress? Any time old code is being changed that doesn't conform to some part of the coding standard is a good time to bring it up to date.
2. Any modified file should have its copyright date updated as well.
3. In test_address.py, it would probably be good to use assertTrue for the equality case, to make it more obvious that both == and != are being tested.
4. It looks like UNIXAddress had some untested behavior - the os.path.samepath check when the two names were not equal. We should try to keep this behavior. Can you add a test exercising it and re-add it? Hopefully FancyEqMixin is still usable, but UNIXAddress.__eq__ probably needs to add the extra samefile check if the inherited __eq__ returns False.
5. This is probably a good time to delete the deprecated __getitem__ and __getslice__ methods on these classes. The change already removes the deprecated comparison to tuple; removing the whole mess at once would be nice.
6. Can you also add two news fragments for this change? One should describe the bug being fixed (comparison of address objects). The other should mention the removed features - tuple comparison and indexing. News fragments are described here.

I checked the patch into a branch. Please make future patches against that branch.

Thanks!

[awclin]

Thanks for the detailed review, and apologies for missing a couple of things that were documented in the developer guidelines first time around. The new patch addresses all the points you've raised.

[exarkun]

(In [30574]) Branching to 'address-not-equal-comparison-4817'

[exarkun]

(In [30575]) Apply 4817-1.patch and 4817-2.patch

refs #4817

[exarkun]

(In [30576]) Apply the rest of them

refs #4817

[exarkun]

Thanks! I noticed a couple more things:

1. _bwHack looks like it's totally obsolete now. I think we can delete the attribute and the related comments. The __init__ parameter should stay, I guess, and be deprecated separately: /
2. test_differentNamesComparison looks like something we should be applying to all of the address types. There could be a buildDifferentAddress hook in addition to buildAddress and the base class could define a test that asserts that the two addresses are not equal to each other.

I checked the patches into a branch, so please make the next patch against that branch. Thanks again!

[awclin]

comments addressed

[awclin]

1st patch against address-not-equal-comparison-4817

[exarkun]

(In [30643]) Branching to 'address-not-equal-comparison-4817-2'

[exarkun]

(In [30645]) Apply 4817-3.patch

refs #4817

[exarkun]

Thanks!

1. Whenever we introduce deprecations, we should make sure that Twisted itself doesn't trigger them (we've failed to do this in the past, and the results are annoyingly visible in the output of trial twisted). So in this case, all of the places in Twisted that instantiate an address should stop passing the final argument.
2. Test method docstrings don't need to say "Assert ..." or "Ensure ...". The docstrings should just explain the feature being tested.
3. The new test methods and helpers should have docstrings as well.
4. UNIXAddressTestCase.buildDifferentAddress should probably return a fixed address, like the IPv4 cases do.
5. lines should be less than 80 columns

[exarkun]

I addressed these points in r30646.

[exarkun]

[Build results  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/address-not-equal-comparison-4817-2]

[exarkun]

(In [30647]) Conditionally define UNIXAddress.eq, only if samefile is defined (so, not on Windows).

refs #4817

[glyph]

The samefile check seems a bit weird to me; won't this change depending on the presence of symlinks and the state of the real filesystem? The TCP addresses don't attempt to determine if their IPs are different interfaces on the same host.

But, I'll trust your judgement if you want to leave it as-is; it otherwise looks fine to merge.

[exarkun]

The samefile doesn't seem too awesome to me, but since it was there before I want to leave it for now. We may want to get rid of it soon though. I don't think it can ever actually be invoked.

[exarkun]

(In [30743]) Merge address-not-equal-comparison-4817-2

Author: awclin, exarkun Reviewer: exarkun, glyph Fixes: #4817

Remove the deprecated support for indexing IPv4Address and UNIXAddress instances. Also deprecate passing the backwards compatibility hack argument to those classes and stop doing it everywhere in Twisted.