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 misleading line in FormalSums #19886

Closed
mantepse opened this issue Jan 14, 2016 · 11 comments
Closed

remove misleading line in FormalSums #19886

mantepse opened this issue Jan 14, 2016 · 11 comments

Comments

@mantepse
Copy link
Contributor

As discussed in this sage-devel discussion, the "elif" branch in this excerpt can never be visited, since if its condition was satisfied, the "if" condition would have been too, and the method would have ended with the "return" statement in the "if" branch.

class FormalSums(UniqueRepresentation, Module):

    Element = FormalSum

    def _element_constructor_(self, x, check=True, reduce=True):

        if isinstance(x, FormalSum):
            P = x.parent()
            if P is self:
                return x
            elif P == self:
                return self.element_class(x._data, check=False, reduce=False, parent=self)
            else:
                x = x._data

We therefore remove the "elif" branch which can only cause confusion.

Component: algebra

Author: Martin Rubey

Branch/Commit: fc1a04f

Reviewer: Daniel Krenn

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

@mantepse mantepse added this to the sage-7.0 milestone Jan 14, 2016
@mantepse
Copy link
Contributor Author

@mantepse
Copy link
Contributor Author

Author: Martin Rubey

@mantepse
Copy link
Contributor Author

Changed branch from u/mantepse/remove_misleading_line_in_formalsums to none

@mantepse
Copy link
Contributor Author

@dkrenn
Copy link
Contributor

dkrenn commented Jan 14, 2016

Reviewer: Daniel Krenn

@dkrenn
Copy link
Contributor

dkrenn commented Jan 14, 2016

Commit: fc1a04f

@dkrenn
Copy link
Contributor

dkrenn commented Jan 14, 2016

comment:4

LGTM


New commits:

fc1a04fremove confusing line

@slel
Copy link
Member

slel commented Jan 14, 2016

comment:5

Can you summarize in one line in the ticket description what this ticket is about?
Why was the line misleading, why removing it is making Sage better?

@mantepse
Copy link
Contributor Author

comment:6

See this thread on sage-devel:

https://groups.google.com/forum/#!topic/sage-devel/09Xf1_coMOc

@slel

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented Jan 20, 2016

Changed branch from u/mantepse/remove_misleading_line_in_formalsums to fc1a04f

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