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

Move methods from Word_nfactor_enumerable to FiniteWord_class #12380

Closed
seblabbe opened this issue Jan 29, 2012 · 14 comments
Closed

Move methods from Word_nfactor_enumerable to FiniteWord_class #12380

seblabbe opened this issue Jan 29, 2012 · 14 comments

Comments

@seblabbe
Copy link
Contributor

The file sage/combinat/words/nfactor_enumerable_word.py and its class Word_nfactor_enumerable were introduced by ticket #8604. Just when it got merged into Sage, I realized it was a mistake. This class should not exist. The fact that its name was ugly was an indice. All those methods should be in a Language class (see #12225). It would get things a lot cleaner.

Only the class FiniteWord_class was inheriting from it, so it farly easy to fix : just move the 14 methods from one class to the other.

Note : this change will be unnoticed by the user.

Apply: attachment: 12380_move_nfactors_methods-sl-updated.patch and attachment: trac_12380_reviewer.patch

Depends on #9958
Depends on #13073

CC: @sagetrac-abmasse @videlec

Component: combinatorics

Author: Sébastien Labbé

Reviewer: André Apitzsch

Merged: sage-5.3.beta2

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

@seblabbe seblabbe added this to the sage-5.3 milestone Jan 29, 2012
@seblabbe seblabbe self-assigned this Jan 29, 2012
@seblabbe
Copy link
Contributor Author

Dependencies: #9958

@seblabbe
Copy link
Contributor Author

comment:1

Depends on one of the patch of #9958 merged in sage-5.0.beta0, namely : 9958_combinat.patch.

@seblabbe

This comment has been minimized.

@seblabbe
Copy link
Contributor Author

Author: Sébastien Labbé

@a-andre
Copy link
Contributor

a-andre commented Jul 14, 2012

comment:3

Your patch looks good. If you are okay with my one, you can change the status to positive review.

@a-andre
Copy link
Contributor

a-andre commented Jul 14, 2012

Reviewer: André Apitzsch

@seblabbe
Copy link
Contributor Author

@seblabbe
Copy link
Contributor Author

Changed dependencies from #9958 to #9958, #13073

@seblabbe
Copy link
Contributor Author

comment:4

Ticket #13073 just changed one line in the file sage/combinat/words/nfactor_enumerable_word.py, so the patch here does not apply anymore. I just updated the patch (under another name) with the following affected changes :

$ diff 12380_move_nfactors_methods-sl.patch 12380_move_nfactors_methods-sl-updated.patch 
426c426
< +        from sage.graphs.all import DiGraph
---
> +        from sage.graphs.digraph import DiGraph
1181c1181
< -        from sage.graphs.all import DiGraph
---
> -        from sage.graphs.digraph import DiGraph

The reviewer's patch still applies over the new updated one. I agree with the changes made by the reviewer. All tests pass in sage/combinat/words. Documentation builds fine. I give a positive review to the reviewer's patch.

Apply 12380_move_nfactors_methods-sl-updated.patch trac_12380_reviewer.patch

Because of changes made for #13073, I will not change the status of the ticket to positive review. I give André a last look at it.

@seblabbe

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Contributor

Merged: sage-5.3.beta2

@jdemeyer
Copy link
Contributor

Attachment: trac_12380_reviewer.patch.gz

Remove trailing white spaces

@jdemeyer
Copy link
Contributor

comment:10

Rebased reviewer patch to sage-5.3.beta1.

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