Opened 6 years ago

Closed 14 months ago

#5124 enhancement closed fixed (fixed)

Remove cmp usage in twisted.names.srvconnect

Reported by: Facundo Batista Owned by: Facundo Batista
Priority: normal Milestone: Python-3.x
Component: names Keywords: py3k
Cc: Thijs Triemstra Branch: branches/names-cmp-5124
branch-diff, diff-cov, branch-cov, buildbot
Author: facundobatista, thijs

Description (last modified by Thijs Triemstra)

The attached patch removes cmp and __cmp__ usage from the following modules:

All the changes are tested (had to add some tests for some changes).

Attachments (4)

remove-cmp-02a.patch (8.9 KB) - added by Facundo Batista 6 years ago.
remove-cmp-02b.patch (7.9 KB) - added by Facundo Batista 6 years ago.
Changes in 'names' only
remove-cmp-02c.patch (7.9 KB) - added by Facundo Batista 6 years ago.
Removed the assertLess calls.
remove-cmp-02d.patch (9.5 KB) - added by Facundo Batista 6 years ago.
Support all comparisons, with more tests

Download all attachments as: .zip

Change History (24)

Changed 6 years ago by Facundo Batista

Attachment: remove-cmp-02a.patch added

comment:1 Changed 6 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Description: modified (diff)

fixup ticket description.

comment:2 Changed 6 years ago by Thijs Triemstra

Component: corenames
Keywords: review removed
Owner: set to Facundo Batista

Thanks for the patch.

compareMarkPos and 'comparePosition' in [trunk/twisted/lore/tree.py] are deprecated since Twisted 9.0, so they shouldn't be updated but removed instead. I guess that needs a new ticket because every removal needs a ticket. So I suggest opening a new one for lore with a patch that removes those deprecated methods, and ignoring the lore changes in the patch on this ticket.

comment:3 Changed 6 years ago by Jean-Paul Calderone

Don't spent time on Lore maintenance. Work on the Sphinx conversion. :)

comment:4 Changed 6 years ago by Facundo Batista

Keywords: review added

thijs: I created ticket #5127 for the removal of these methods, and attached a new patch here with the changes in names only. Thanks!

exarkun: I don't want to spend any time in Lore, but I need those cmp out of the way for Py3k :).

Changed 6 years ago by Facundo Batista

Attachment: remove-cmp-02b.patch added

Changes in 'names' only

comment:5 in reply to:  4 Changed 6 years ago by Thijs Triemstra

Description: modified (diff)
Summary: Remove cmp usage in 'lore' and 'names'Remove cmp usage in 'names'

Replying to facundobatista:

thijs: I created ticket #5127 for the removal of these methods, and attached a new patch here with the changes in names only. Thanks!

Thanks, I'll update the ticket title and summary to reflect this update.

comment:6 Changed 6 years ago by therve

Owner: Facundo Batista deleted

comment:7 Changed 6 years ago by Thijs Triemstra

Milestone: Python-3.x

comment:8 Changed 6 years ago by Thijs Triemstra

Author: thijs
Branch: branches/names-cmp-5124

(In [32112]) Branching to 'names-cmp-5124'

comment:9 Changed 6 years ago by Thijs Triemstra

(In [32113]) apply remove-cmp-02b.patch, add news file. refs #5124

comment:10 Changed 6 years ago by Thijs Triemstra

(In [32114]) pyflakes and coding std fixes, refs #5124

comment:11 Changed 6 years ago by Thijs Triemstra

Author: thijsfacundobatista, thijs
Keywords: review removed
Owner: set to Facundo Batista

The build results show 3 failing tests. Can you make new patches against this branch?

Changed 6 years ago by Facundo Batista

Attachment: remove-cmp-02c.patch added

Removed the assertLess calls.

comment:12 Changed 6 years ago by Facundo Batista

Keywords: review added

Attached the 'c' version of the patch, where I removed the assertLess calls (that were only 2.7, sorry).

comment:13 Changed 6 years ago by Facundo Batista

Owner: Facundo Batista deleted

comment:14 Changed 6 years ago by Glyph

Keywords: review removed
Owner: set to Facundo Batista

It appears that this patch has a glaring omission: to wit, tests for > comparison. Is there a rule about __gt__ that it automatically works if __lt__, __eq__ and __ne__ are all implemented? In any case I think more test coverage is required...

comment:15 Changed 6 years ago by Facundo Batista

Keywords: review added
Owner: Facundo Batista deleted

Yes, gt automatically works if lt is implemented and both are the same type (because of reflection when doing the comparison... Python doesn't find gt on one object, tries lt on the other).

However, the <= comparisons weren't working... I added code for that, and tests cases for everything.

'c' version of the patch is attached.

comment:16 Changed 6 years ago by Facundo Batista

Bah, sorry, the version of the patch for these last improvements is the 'd' one.

Changed 6 years ago by Facundo Batista

Attachment: remove-cmp-02d.patch added

Support all comparisons, with more tests

comment:17 Changed 6 years ago by Dustin J. Mitchell

Keywords: review removed
Owner: set to Facundo Batista

In twisted/names/dns.py, the fallback for le uses < instead of <=. The effect is negligible - a Query is never equal to anything else, so <= and < are equivalent, but the difference stands out in the code and looks suspicious, so it should be corrected. On that note, what is the point of self.__class__ == other.__class__ if you already know that other is not an instance of Query? Why not just return False from __eq___ in that case?

I can't see any other problems - all of the comments in the previous reviews seem to be addressed.

comment:18 Changed 6 years ago by Thijs Triemstra

Description: modified (diff)

#5348 was marked as a duplicate.

comment:19 Changed 5 years ago by Thijs Triemstra

Description: modified (diff)
Summary: Remove cmp usage in 'names'Remove cmp usage in twisted.names.srvconnect

The first part of this ticket (twisted.names.dns) was fixed with #6059.

comment:20 Changed 14 months ago by Ralph Meijer <ralphm@…>

Resolution: fixed
Status: newclosed

In 5aee182:

Merge py3-srvconnect-8262: Port twisted.names.srvconnect to Python 3.

Author: ralphm
Reviewer: adiroiban, hawkowl, glyph
Fixes: #8262, #5124, #2800

This also cleans up the use of a cmp function for sorting SRV records,
makes the pickServer implementation in line with the algorithm
specified in the RFC and adds tests around that to match.

Note: See TracTickets for help on using tickets.