Opened 4 years ago

Closed 4 years ago

#22815 closed enhancement (fixed)

py3: caring for cmp in combinat/combinat

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.0
Component: python3 Keywords:
Cc: tscrim, jdemeyer, aapitzsch Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 58ff8a2 (Commits, GitHub, GitLab) Commit: 58ff8a20916adf6482e2cde50e2398987d80fc22
Dependencies: Stopgaps:

Status badges

Description

getting rid of __cmp__ there

Change History (11)

comment:1 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/22815
  • Cc tscrim jdemeyer aapitzsch added
  • Commit set to 1f32d6989e76bdf5b73a9cb24c112f2ad3403471
  • Status changed from new to needs_review

New commits:

1f32d69care for cmp in combinat/combinat

comment:2 Changed 4 years ago by tscrim

  • Component changed from PLEASE CHANGE to python3
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:3 Changed 4 years ago by chapoton

  • Status changed from positive_review to needs_work

some failing doctests

comment:4 Changed 4 years ago by tscrim

Hmm...I'm guessing the problem comes from the implementation of __cmp__ (or __richcmp__) in Element? Fundamentally we don't have to remove __cmp__ as the rich comparisons are implemented for CombinatorialObject (and are compatible). Although this does expose that the rich comparisons are not really tested in the subclasses...

comment:5 Changed 4 years ago by chapoton

I was trying to fix one of the doctest failures in the last patchbot run on #22297.

comment:6 Changed 4 years ago by git

  • Commit changed from 1f32d6989e76bdf5b73a9cb24c112f2ad3403471 to 58ff8a20916adf6482e2cde50e2398987d80fc22

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

da82a54Merge branch 'u/chapoton/22815' in 8.0.b4
58ff8a2trac 22815 remove one call to cmp

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

Travis, do you think #22642 will fix the remaining failure here ?

comment:8 in reply to: ↑ 7 Changed 4 years ago by tscrim

  • Status changed from needs_work to needs_review

Replying to chapoton:

Travis, do you think #22642 will fix the remaining failure here ?

Sorry, completely forgot about this. From the patchbot, it looks like it did.

comment:9 Changed 4 years ago by chapoton

yes. So maybe we can set to positive ?

comment:10 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:11 Changed 4 years ago by vbraun

  • Branch changed from u/chapoton/22815 to 58ff8a20916adf6482e2cde50e2398987d80fc22
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.