Opened 6 years ago

Closed 6 years ago

#21247 closed enhancement (fixed)

py3 remove __cmp__ in lazy_import.pyx

Reported by: chapoton Owned by:
Priority: major Milestone: sage-7.4
Component: python3 Keywords:
Cc: tscrim, jdemeyer, aapitzsch Merged in:
Authors: Frédéric Chapoton, Jeroen Demeyer Reviewers: Jeroen Demeyer, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 004d759 (Commits, GitHub, GitLab) Commit: 004d7591f2dbcbd33fb0ee94e8786d98c221e4bc
Dependencies: Stopgaps:

Status badges

Description

as a tiny step towards python3, let us avoid using cmp for lazy import.

Let us just try to delete __cmp__ from this file. There remains __richcmp__.

Change History (22)

comment:1 Changed 6 years ago by chapoton

  • Branch set to public/21247
  • Commit set to 8a911adb7e4a9d12051aefa6704c515a6aeac3cf
  • Status changed from new to needs_review

New commits:

8a911adremove __cmp__ (with cmp inside) in lazy_import.pyx

comment:2 Changed 6 years ago by jdemeyer

The __cmp__ in lazy_import.pyx should stay because it is part of a framework (lazy imports) and not an individual class.

So I propose to close this as invalid/wontfix.

comment:3 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_info

comment:4 follow-up: Changed 6 years ago by chapoton

But is this really needed ?

Does removing it break something ?

How to get rid of the cmp inside this method ?

I may be doing stupid things, but your very short comments are not really helpful.

comment:5 in reply to: ↑ 4 Changed 6 years ago by jdemeyer

Replying to chapoton:

I may be doing stupid things, but your very short comments are not really helpful.

I'm writing short comments mainly because I have not thought very much about it. I'd rather write a short comment now with the idea that we can still think about it or discuss later.

comment:6 Changed 6 years ago by jdemeyer

Here is a more concrete proposal: keep this cmp for now and deal with it later, when most/all of the other files have been converted to use Python 3 comparisons.

comment:7 Changed 6 years ago by chapoton

patchbot is green (except for timeout failure in Tutte polynomials, which is a disease of this patchbot)

comment:8 Changed 6 years ago by chapoton

ping ?

comment:9 follow-up: Changed 6 years ago by chapoton

  • Cc tscrim jdemeyer aapitzsch added
  • Status changed from needs_info to needs_review

I would like to propose this for review again, as this does not break anything.

Maybe we cannot allow to say "let us not remove this cmp now, wait until all the others have been removed" about every instance of cmp, after all. :)

comment:10 in reply to: ↑ 9 Changed 6 years ago by jdemeyer

Replying to chapoton:

Maybe we cannot allow to say "let us not remove this cmp now, wait until all the others have been removed" about every instance of cmp, after all. :)

Not every instance of cmp(), just this one specifically.

comment:11 Changed 6 years ago by chapoton

Could you please explain why you think this should not be done now ?

  • Does "being part of a framework" means something precise ?
  • The bot is green, so this does not break anything.
  • We remove __cmp__ but there is still __richcmp__.
Last edited 6 years ago by chapoton (previous) (diff)

comment:12 Changed 6 years ago by jdemeyer

Since you seem to be insisting a lot, here is a possible compromise:

def __cmp__(self, other):
    raise NotImplementedError("Old-style comparisons are not supported for lazily imported objects")

That will raise an exception when somebody tries to use this __cmp__. So if you break something, you will break it explicitly and people don't have to figure out that a subtle change in lazy imports broke their use case.

If this passes patchbot testing, so you can set the ticket to positive_review.

comment:13 Changed 6 years ago by chapoton

  • Branch changed from public/21247 to u/chapoton/21247
  • Commit changed from 8a911adb7e4a9d12051aefa6704c515a6aeac3cf to 75b86808bf0476f9bd9b108a7477b3558f7ccf62

Sorry for insisting so much, but I am really bothered by not being able to compile all Cython files with py3. And the "cmp()" problem is the first obstacle. So the road must go through getting rid of all "cmp()" in pyx files, with no exception.

Let us see what the patchbot says.


New commits:

75b8680trac 21247 raising NotImplementedError for old-style cmp in lazy_import

comment:14 Changed 6 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

Can you keep the doctest please (showing that the exception is indeed raised).

comment:15 Changed 6 years ago by chapoton

In fact, the doctest just works, showing that this method is never called. What should I do ?

comment:16 Changed 6 years ago by jdemeyer

I will have a look.

comment:17 Changed 6 years ago by jdemeyer

Here is the explanation: cmp(x,y) only calls x.__cmp__(y) if the types of x and y are equal. So we need both x and y to be a lazy import.

comment:18 Changed 6 years ago by jdemeyer

  • Branch changed from u/chapoton/21247 to u/jdemeyer/21247

comment:19 Changed 6 years ago by jdemeyer

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, Jeroen Demeyer
  • Commit changed from 75b86808bf0476f9bd9b108a7477b3558f7ccf62 to 004d7591f2dbcbd33fb0ee94e8786d98c221e4bc
  • Status changed from needs_work to needs_review

New commits:

004d759Raise NotImplementedError for old-style cmp in lazy_import

comment:20 Changed 6 years ago by chapoton

Ok, looks good to me, and bot is green. Jeroen, if you agree, please set to positive review.

comment:21 Changed 6 years ago by jdemeyer

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Frédéric Chapoton
  • Status changed from needs_review to positive_review

comment:22 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/21247 to 004d7591f2dbcbd33fb0ee94e8786d98c221e4bc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.