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

Remove support for Parent.__init__(gens=...) #21385

Closed
jdemeyer opened this issue Sep 1, 2016 · 9 comments
Closed

Remove support for Parent.__init__(gens=...) #21385

jdemeyer opened this issue Sep 1, 2016 · 9 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Sep 1, 2016

Parent.__init__() takes a gens keyword argument, but it turns out that nothing in Sage actually uses this argument. So we simply remove it. Then we can also remove the _populate_generators_ method which is never actually called.

(see also the task ticket: #21380)

Depends on #21382

CC: @videlec @tscrim

Component: categories

Author: Jeroen Demeyer

Branch/Commit: 517808d

Reviewer: Marc Mezzarobba

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

@jdemeyer jdemeyer added this to the sage-7.4 milestone Sep 1, 2016
@jdemeyer
Copy link
Author

jdemeyer commented Sep 1, 2016

Branch: u/jdemeyer/ticket/21385

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2016

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

517808dRemove support for Parent.__init__(gens=...)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2016

Commit: 517808d

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Remove _populate_generators_ Remove support for Parent.__init__(gens=...) Sep 1, 2016
@mezzarobba
Copy link
Member

comment:4

The changes look good to me, but shouldn't the feature in question first be deprecated, in case something outside Sage uses it? (To be clear, I personally think the deprecation policy is both too vague and much too strict for the level of quality and maturity of large parts of Sage. But Sage does have such a policy...)

@jdemeyer
Copy link
Author

comment:5

Replying to @mezzarobba:

The changes look good to me, but shouldn't the feature in question first be deprecated, in case something outside Sage uses it?

I don't see the point of deprecation here because the gens do not work anyway. Currently, the gens are passed to _populate_generators_ which assigns a _generators attribute which is then never used.

So I am not removing any functionality, I am basically just removing dead code.

In any case, this ticket depends on #21382 which should be reviewed first.

@mezzarobba
Copy link
Member

comment:6

Replying to @jdemeyer:

I don't see the point of deprecation here because the gens do not work anyway. Currently, the gens are passed to _populate_generators_ which assigns a _generators attribute which is then never used.

So I am not removing any functionality, I am basically just removing dead code.

Fine then. (I hadn't paid attention to the fact that Parent.__init__() also accepted **kwds, sorry.)

In any case, this ticket depends on #21382 which should be reviewed first.

Yes, I posted the comment here since it applied to the second commit, but I was actually waiting for your answer to set the first ticket to positive review :-).

@mezzarobba
Copy link
Member

Reviewer: Marc Mezzarobba

@vbraun
Copy link
Member

vbraun commented Sep 19, 2016

Changed branch from u/jdemeyer/ticket/21385 to 517808d

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