Opened 3 years ago

Last modified 23 months ago

#5124 enhancement new

Remove cmp usage in twisted.names.srvconnect

Reported by: facundobatista Owned by: facundobatista
Priority: normal Milestone: Python-3.x
Component: names Keywords: py3k
Cc: thijs Branch: branches/names-cmp-5124
(diff, github, buildbot, log)
Author: facundobatista, thijs Launchpad Bug:

Description (last modified by thijs)

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 facundobatista 3 years ago.
remove-cmp-02b.patch (7.9 KB) - added by facundobatista 3 years ago.
Changes in 'names' only
remove-cmp-02c.patch (7.9 KB) - added by facundobatista 3 years ago.
Removed the assertLess calls.
remove-cmp-02d.patch (9.5 KB) - added by facundobatista 3 years ago.
Support all comparisons, with more tests

Download all attachments as: .zip

Change History (23)

Changed 3 years ago by facundobatista

comment:1 Changed 3 years ago by thijs

  • Cc thijs added
  • Description modified (diff)

fixup ticket description.

comment:2 Changed 3 years ago by thijs

  • Component changed from core to names
  • Keywords review removed
  • Owner set to facundobatista

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 3 years ago by exarkun

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

comment:4 follow-up: Changed 3 years ago by facundobatista

  • 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 3 years ago by facundobatista

Changes in 'names' only

comment:5 in reply to: ↑ 4 Changed 3 years ago by thijs

  • Description modified (diff)
  • Summary changed from Remove cmp usage in 'lore' and 'names' to 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 3 years ago by therve

  • Owner facundobatista deleted

comment:7 Changed 3 years ago by thijs

  • Milestone set to Python-3.x

comment:8 Changed 3 years ago by thijs

  • Author set to thijs
  • Branch set to branches/names-cmp-5124

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

comment:9 Changed 3 years ago by thijs

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

comment:10 Changed 3 years ago by thijs

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

comment:11 Changed 3 years ago by thijs

  • Author changed from thijs to facundobatista, thijs
  • Keywords review removed
  • Owner set to facundobatista

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

Changed 3 years ago by facundobatista

Removed the assertLess calls.

comment:12 Changed 3 years ago by facundobatista

  • 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 3 years ago by facundobatista

  • Owner facundobatista deleted

comment:14 Changed 3 years ago by glyph

  • Keywords review removed
  • Owner set to facundobatista

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 3 years ago by facundobatista

  • Keywords review added
  • Owner facundobatista 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 3 years ago by facundobatista

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

Changed 3 years ago by facundobatista

Support all comparisons, with more tests

comment:17 Changed 3 years ago by dustin

  • Keywords review removed
  • Owner set to facundobatista

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 3 years ago by thijs

  • Description modified (diff)

#5348 was marked as a duplicate.

comment:19 Changed 23 months ago by thijs

  • Description modified (diff)
  • Summary changed from Remove cmp usage in 'names' to Remove cmp usage in twisted.names.srvconnect

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

Note: See TracTickets for help on using tickets.