Opened 5 years ago
Closed 5 years ago
#22981 closed enhancement (fixed)
py3: get rid of cmp() in element.pyx
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  python3  Keywords:  
Cc:  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Jeroen Demeyer, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  89b2762 (Commits, GitHub, GitLab)  Commit:  89b27622e668b1bd03b12a7aadb59327ef4cec30 
Dependencies:  Stopgaps: 
Description
So that this file cythonize correctly with python3
This will be handled more seriously in #22297 later.
Change History (21)
comment:1 Changed 5 years ago by
 Branch set to u/chapoton/22981
 Commit set to 185d9921ffb683b477151c6f59e04342531833c8
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
You have changed what you are comparing. Before, it was the coerced objects cmp(left, right)
, but now you are only comparing the types of the objects if tl < tr:
. I believe this should be if left < right
.
comment:3 Changed 5 years ago by
 Commit changed from 185d9921ffb683b477151c6f59e04342531833c8 to 0ff20a57b90b803355587bda6bfddf1d85a133d6
Branch pushed to git repo; I updated commit sha1. New commits:
0ff20a5  trac 22981 correct wrong comparison

comment:4 Changed 5 years ago by
right, sorry for that. Correction done
comment:5 Changed 5 years ago by
 Commit changed from 0ff20a57b90b803355587bda6bfddf1d85a133d6 to 0a878d6dfc19635e776749c5ff769679c7ddbac1
Branch pushed to git repo; I updated commit sha1. New commits:
0a878d6  trac 22981 simplify

comment:6 Changed 5 years ago by
and simplified a bit
comment:7 Changed 5 years ago by
 Reviewers set to Travis Scrimshaw
If the patchbot comes back green, then you can set a positive review.
comment:8 Changed 5 years ago by
 Status changed from needs_review to positive_review
ok, bot is essentially green, setting to positive
comment:9 Changed 5 years ago by
 Branch u/chapoton/22981 deleted
 Commit 0a878d6dfc19635e776749c5ff769679c7ddbac1 deleted
 Milestone changed from sage8.0 to sageduplicate/invalid/wontfix
 Reviewers changed from Travis Scrimshaw to Jeroen Demeyer
Green bot does not mean that the ticket makes sense! This is not going to help us porting to Python 3, see #22297 for a long discussion.
comment:10 Changed 5 years ago by
See also #22029 (which contains the long discussion that I meant).
comment:11 Changed 5 years ago by
Oh, come on, Jeroen, please. Could you please explain why you think that this does not make sense, instead of changing the milestone to invalid like that ?
By the way, this ticket is not meant as a replacement to #22297, but as a temporary fix. I am still planning to do the job at #22297. But I also would like to have all pyx files fixed. This can be achieved here at no cost and I do not understand why you object so strongly.
comment:12 Changed 5 years ago by
Could you try with a raise NotImplementedError
instead of the old code calling cmp()
, similar that what we did for lazy imports in #21247.
comment:13 Changed 5 years ago by
 Branch set to public/22981v2
 Commit set to 89b27622e668b1bd03b12a7aadb59327ef4cec30
comment:14 Changed 5 years ago by
 Status changed from positive_review to needs_review
comment:15 Changed 5 years ago by
 Milestone changed from sageduplicate/invalid/wontfix to sage8.0
comment:16 Changed 5 years ago by
Thinking more about it, the original branch wasn't so bad. It just replaces one call to cmp()
with almost equivalent Python code. So I retract my comment 9.
Still, if the raise NotImplementError
works, that would be even better since it would be one less case to worry about in future tickets.
comment:17 Changed 5 years ago by
bot is essentially green; please review
comment:18 Changed 5 years ago by
ping ?
comment:20 Changed 5 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Travis Scrimshaw
 Status changed from needs_review to positive_review
Jeroen, if you have any more objections, then feel free to revert the positive review.
comment:21 Changed 5 years ago by
 Branch changed from public/22981v2 to 89b27622e668b1bd03b12a7aadb59327ef4cec30
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
py3: get rid of cmp() in element.pyx