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

Simplify _populate_generators_ #21381

Closed
jdemeyer opened this issue Aug 31, 2016 · 10 comments
Closed

Simplify _populate_generators_ #21381

jdemeyer opened this issue Aug 31, 2016 · 10 comments

Comments

@jdemeyer
Copy link

  1. The argument category is never used.

  2. gens is never None.

  3. We can get rid of _ngens_ which appears only in _populate_generators_ but is never actually called.

(see also the task ticket: #21380)

CC: @videlec @tscrim

Component: categories

Author: Jeroen Demeyer

Branch/Commit: 079a1d4

Reviewer: Travis Scrimshaw

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

@jdemeyer jdemeyer added this to the sage-7.4 milestone Aug 31, 2016
@videlec

This comment has been minimized.

@jdemeyer
Copy link
Author

@jdemeyer
Copy link
Author

Commit: 079a1d4

@jdemeyer
Copy link
Author

New commits:

079a1d4Simplify _populate_generators_

@tscrim
Copy link
Collaborator

tscrim commented Aug 31, 2016

comment:5

Could move the entire logic into that branch Parent. _populate_generators_ is only called there and never overriden/used elsewhere?

@videlec
Copy link
Contributor

videlec commented Aug 31, 2016

comment:6

Replying to @tscrim:

Could move the entire logic into that branch Parent. _populate_generators_ is only called there and never overriden/used elsewhere?

I would prefer keeping all the generators stuff into one place (for now CategoryObject). Starting splitting it between CategoryObject and Parent will make it harder for #21380.

@jdemeyer
Copy link
Author

comment:7

Replying to @tscrim:

Could move the entire logic into that branch Parent. _populate_generators_ is only called there and never overriden/used elsewhere?

Can we stick to just the contents of this ticket? We can still move it later.

@tscrim
Copy link
Collaborator

tscrim commented Aug 31, 2016

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 31, 2016

comment:8

Alright.

@vbraun
Copy link
Member

vbraun commented Sep 4, 2016

Changed branch from u/jdemeyer/simplify__populate_generators_ to 079a1d4

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