Skip to content

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Dec 11, 2017

Fix #4340 by considering TupleExpr as the only lvalue sequence.

(And maybe one day it will be LvalueSeq)

@msullivan
Copy link
Collaborator

Could you add a python2 test and a test with a list lvalue and a Tuple annotation?

@elazarg elazarg force-pushed the tuple-lvalue branch 3 times, most recently from 96ac9e1 to c06d2ea Compare January 10, 2018 08:59
@elazarg
Copy link
Contributor Author

elazarg commented Jan 10, 2018

Thanks! Indeed there was a Python2 bug :)

@elliott-beach
Copy link
Contributor

For what it's worth, I think this is a cool change that should be merged.

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

One minor thing.

return ListExpr([self.visit(e) for e in n.elts])
def visit_List(self, n: ast3.List) -> Union[ListExpr, TupleExpr]:
expr_list = [self.visit(e) for e in n.elts] # type: List[Expression]
if isinstance(n.ctx, ast3.Store):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a quick comment here and in fastparse2 explaining why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@msullivan msullivan merged commit d8b884f into python:master Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants