Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

py3 get rid of cmp() in btquotient.py #22267

Closed
fchapoton opened this issue Jan 27, 2017 · 12 comments
Closed

py3 get rid of cmp() in btquotient.py #22267

fchapoton opened this issue Jan 27, 2017 · 12 comments

Comments

@fchapoton
Copy link
Contributor

as another step to py3

CC: @tscrim @a-andre @jdemeyer

Component: python3

Author: Frédéric Chapoton

Branch/Commit: b6d14fe

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/22267

@fchapoton fchapoton added this to the sage-7.6 milestone Jan 27, 2017
@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/22267

@fchapoton
Copy link
Contributor Author

New commits:

c156f33py3 getting rid of cmp() in btquotient.py

@fchapoton
Copy link
Contributor Author

Commit: c156f33

@fchapoton
Copy link
Contributor Author

comment:2

Triggers one doctest failure. Damn.

Maybe one needs to implement __ne__ ?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 29, 2017

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

b6d14fetrac 22267 adding the missing __ne__

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 29, 2017

Changed commit from c156f33 to b6d14fe

@fchapoton
Copy link
Contributor Author

comment:4

waiting for the bots

@fchapoton
Copy link
Contributor Author

comment:5

green bot, please review (easy one)

@tscrim
Copy link
Collaborator

tscrim commented Jan 30, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jan 30, 2017

comment:6

Since there were no tests for checking the inequalities, I think we are okay in assuming that was never really used (i.e., the total ordering was more artificial because of the __cmp__ definition). While I am slightly worried someone is using it in an essential way, not enough to keep this from setting this to a positive review to continue the march to Python3.

@fchapoton
Copy link
Contributor Author

comment:7

thanks ! what do you think of #22256 ?

@vbraun
Copy link
Member

vbraun commented Feb 2, 2017

Changed branch from u/chapoton/22267 to b6d14fe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants