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

Change CompositConstructionFunctor to CompositeConstructionFunctor #10318

Closed
JohnCremona opened this issue Nov 24, 2010 · 7 comments
Closed

Change CompositConstructionFunctor to CompositeConstructionFunctor #10318

JohnCremona opened this issue Nov 24, 2010 · 7 comments

Comments

@JohnCremona
Copy link
Member

I think that CompositConstructionFunctor should be CompositeConstructionFunctor.

Component: categories

Author: John Cremona

Reviewer: David Kirkby

Merged: sage-4.6.2.alpha0

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

@JohnCremona
Copy link
Member Author

Attachment: trac_10318-Composit.patch.gz

Applies to 4.6.1.alpha2 + patch from #8807

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Nov 24, 2010

comment:2

I'm a bit puzzled why the need for two patches. Is the combined patch one in which could be used instead of the patch on #8807?

I have no problems giving the small patch a positive reivew. I guess its up to the release manager how best to handle this - whether #8807 is applied first, and then your simple patch, or just your combined patch.

I would have thought the best way to handle this was to just have the simple patch, and make it conditional on #8807 being applied first.

Dave

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Nov 24, 2010

Reviewer: David Kirkby

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Nov 24, 2010

comment:3

Ignore what I said - I see this patch just does many identical spelling corrections - I assumed initially it was just one spelling correction, along with the whole contents of another patch, which made less sence

@JohnCremona
Copy link
Member Author

comment:4

That's right -- I made it conditional on the #8807 patch since that patch has the word in it, and it also makes a lot of changes to the file where most of the occurrences occur.

I'll put a cross-reference at #8807 saying that this ticket has a positive review and can/should be merged right after that one.

@jdemeyer jdemeyer modified the milestones: sage-4.6.1, sage-4.6.2 Nov 26, 2010
@simon-king-jena
Copy link
Member

comment:6

depends on #8807

@jdemeyer
Copy link
Contributor

Merged: sage-4.6.2.alpha0

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