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.

Sign up for GitHub

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

Jump to bottom

sort using key instead of cmp in infinite polynomials #21035

Closed
fchapoton opened this issue Jul 17, 2016 · 12 comments
Closed

sort using key instead of cmp in infinite polynomials #21035

fchapoton opened this issue Jul 17, 2016 · 12 comments
Labels
c: python3 p: major / 3 t: enhancement
Milestone
sage-7.3

Comments

@fchapoton
Copy link
Contributor

as a modest step towards py3

CC: @tscrim @jm58660 @jdemeyer

Component: python3

Author: Frédéric Chapoton

Branch/Commit: 2ef77ae

Reviewer: Jori Mäntysalo

Issue created by migration from https://trac.sagemath.org/ticket/21035
@fchapoton fchapoton added this to the sage-7.3 milestone Jul 17, 2016
@fchapoton
Copy link
Contributor Author

Branch: public/21035

@fchapoton
Copy link
Contributor Author

New commits:

78b457atrac 21035 sort using key in infinite polynomial rings

@fchapoton
Copy link
Contributor Author

Commit: 78b457a

@a-andre
Copy link

a-andre commented Jul 17, 2016

comment:2

Shouldn't varname_cmp(self,x,y) be deprecated for some time before removing it?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2016

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

2ef77aetra 21035 deprecate varname_cmp

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2016

Changed commit from 78b457a to 2ef77ae

@fchapoton
Copy link
Contributor Author

comment:4

done

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 18, 2016

comment:5

This seems to be OK, tests passed and maybe I could give a positive review.

But is this meant to be auxiliarity function only? If so, why it's name does not begin with _?

I don't know if deprecation is even needed, but it don't hurt.

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 18, 2016

comment:6

Feel free to mark as positive_review if you think that underscore prefix at function name is not needed. But if you change it, remember to change also deprecation warning to point the new one.

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 18, 2016

Reviewer: Jori Mäntysalo

@fchapoton
Copy link
Contributor Author

comment:7

Thanks Jori. I will keep this ticket as it is, because py3 is still a long way ahead
and I want to advance.

@vbraun
Copy link
Member

vbraun commented Jul 19, 2016

Changed branch from public/21035 to 2ef77ae

@vbraun vbraun closed this as completed in 6ecfc85 Jul 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: python3 p: major / 3 t: enhancement
Projects
None yet
Milestone
sage-7.3
Development

No branches or pull requests
3 participants
@vbraun @fchapoton @a-andre