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: sage-8.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:

Status badges

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 chapoton

  • Branch set to u/chapoton/22981
  • Commit set to 185d9921ffb683b477151c6f59e04342531833c8
  • Status changed from new to needs_review

New commits:

185d992py3: get rid of cmp() in element.pyx

comment:2 Changed 5 years ago by tscrim

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 git

  • Commit changed from 185d9921ffb683b477151c6f59e04342531833c8 to 0ff20a57b90b803355587bda6bfddf1d85a133d6

Branch pushed to git repo; I updated commit sha1. New commits:

0ff20a5trac 22981 correct wrong comparison

comment:4 Changed 5 years ago by chapoton

right, sorry for that. Correction done

comment:5 Changed 5 years ago by git

  • Commit changed from 0ff20a57b90b803355587bda6bfddf1d85a133d6 to 0a878d6dfc19635e776749c5ff769679c7ddbac1

Branch pushed to git repo; I updated commit sha1. New commits:

0a878d6trac 22981 simplify

comment:6 Changed 5 years ago by chapoton

and simplified a bit

comment:7 Changed 5 years ago by tscrim

  • 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 chapoton

  • Status changed from needs_review to positive_review

ok, bot is essentially green, setting to positive

comment:9 Changed 5 years ago by jdemeyer

  • Authors Frédéric Chapoton deleted
  • Branch u/chapoton/22981 deleted
  • Commit 0a878d6dfc19635e776749c5ff769679c7ddbac1 deleted
  • Milestone changed from sage-8.0 to sage-duplicate/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 jdemeyer

See also #22029 (which contains the long discussion that I meant).

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:11 Changed 5 years ago by chapoton

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 jdemeyer

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 chapoton

  • Branch set to public/22981v2
  • Commit set to 89b27622e668b1bd03b12a7aadb59327ef4cec30

like that ?


New commits:

89b2762trac 22981 another try: replace cmp() by raise

comment:14 Changed 5 years ago by chapoton

  • Status changed from positive_review to needs_review

comment:15 Changed 5 years ago by chapoton

  • Milestone changed from sage-duplicate/invalid/wontfix to sage-8.0

comment:16 Changed 5 years ago by jdemeyer

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 chapoton

bot is essentially green; please review

comment:18 Changed 5 years ago by chapoton

ping ?

comment:19 Changed 5 years ago by chapoton

  • Authors set to Frédéric Chapoton

ping ?

comment:20 Changed 5 years ago by tscrim

  • 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 vbraun

  • Branch changed from public/22981v2 to 89b27622e668b1bd03b12a7aadb59327ef4cec30
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.