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

deprecate some cmp keyword in favor of key in factorizations #21145

Closed
fchapoton opened this issue Aug 1, 2016 · 21 comments
Closed

deprecate some cmp keyword in favor of key in factorizations #21145

fchapoton opened this issue Aug 1, 2016 · 21 comments

Comments

@fchapoton
Copy link
Contributor

as a step towards python3

let us try to get rid of cmp= in factorizations

CC: @tscrim @jm58660 @jdemeyer

Component: python3

Author: Frédéric Chapoton

Branch/Commit: 930414a

Reviewer: Travis Scrimshaw

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

@fchapoton fchapoton added this to the sage-7.3 milestone Aug 1, 2016
@fchapoton
Copy link
Contributor Author

Branch: public/21145

@fchapoton
Copy link
Contributor Author

Author: Frédéric Chapoton

@fchapoton
Copy link
Contributor Author

New commits:

6406454trac 21145 first tentative

@fchapoton
Copy link
Contributor Author

Commit: 6406454

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2016

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

3b72723trac 21145 more work on deprecating cmp in factorisations

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2016

Changed commit from 6406454 to 3b72723

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2016

Changed commit from 3b72723 to d23c60a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2016

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

d23c60ano change to ordered tree (moved to another ticket)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2016

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

24f0320trac 21145 work on cmp in factorisations

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2016

Changed commit from d23c60a to 24f0320

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2016

Changed commit from 24f0320 to d2531cf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 2, 2016

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

d2531cftrac #21145 fixing one failing doctest

@fchapoton
Copy link
Contributor Author

comment:7

bot is green !

@a-andre
Copy link

a-andre commented Aug 2, 2016

comment:8

Calls like self.__x.sort(cmp=_cmp) will crash in python 3. I suggest you use cmp_to_key() from functools:
self.__x.sort(key=cmp_to_key(_cmp))

@tscrim
Copy link
Collaborator

tscrim commented Aug 3, 2016

comment:9

LGTM.

@a-andre Those should go away by the time we switch to Python3 (hopefully we will be there before EOL for Python2).

@tscrim
Copy link
Collaborator

tscrim commented Aug 3, 2016

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 3, 2016

comment:10

Actually, I now understand Andre's comment, we should change

-self.__x.sort(cmp=_cmp)
+self.__x.sort(key=cmp_to_key(_cmp))

in the deprecated code to be python2/3 compatible now (I interpreted it as a long-term, sorry!).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2016

Changed commit from d2531cf to 930414a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 3, 2016

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

930414atrac #21145 using cmp_to_key in deprecated code

@tscrim
Copy link
Collaborator

tscrim commented Aug 3, 2016

comment:13

Thanks, and sorry I didn't understand it earlier.

@vbraun
Copy link
Member

vbraun commented Aug 7, 2016

Changed branch from public/21145 to 930414a

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

4 participants