Opened 7 years ago
Closed 23 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 )
The attached patch removes cmp and __cmp__ usage from the following modules:
- twisted/names/dns.py
twisted/names/srvconnect.py
All the changes are tested (had to add some tests for some changes).
Attachments (4)
Change History (24)
Changed 7 years ago by
| Attachment: | remove-cmp-02a.patch added |
|---|
comment:1 Changed 7 years ago by
| Cc: | Thijs Triemstra added |
|---|---|
| Description: | modified (diff) |
comment:2 Changed 7 years ago by
| Component: | core → names |
|---|---|
| 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 7 years ago by
Don't spent time on Lore maintenance. Work on the Sphinx conversion. :)
comment:4 follow-up: 5 Changed 7 years ago by
| 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 :).
comment:5 Changed 7 years ago by
| 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
namesonly. Thanks!
Thanks, I'll update the ticket title and summary to reflect this update.
comment:6 Changed 7 years ago by
| Owner: | Facundo Batista deleted |
|---|
comment:7 Changed 7 years ago by
| Milestone: | → Python-3.x |
|---|
comment:8 Changed 7 years ago by
| Author: | → thijs |
|---|---|
| Branch: | → branches/names-cmp-5124 |
(In [32112]) Branching to 'names-cmp-5124'
comment:11 Changed 7 years ago by
| Author: | thijs → facundobatista, thijs |
|---|---|
| Keywords: | review removed |
| Owner: | set to Facundo Batista |
The build results show 3 failing tests. Can you make new patches against this branch?
comment:12 Changed 7 years ago by
| 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 7 years ago by
| Owner: | Facundo Batista deleted |
|---|
comment:14 Changed 7 years ago by
| 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 7 years ago by
| 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 7 years ago by
Bah, sorry, the version of the patch for these last improvements is the 'd' one.
Changed 7 years ago by
| Attachment: | remove-cmp-02d.patch added |
|---|
Support all comparisons, with more tests
comment:17 Changed 7 years ago by
| 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:19 Changed 6 years ago by
| 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.
fixup ticket description.