Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Added matrix multiplication operator handling #246

Merged
merged 3 commits into from
Apr 17, 2017

Conversation

Nurdok
Copy link
Member

@Nurdok Nurdok commented Apr 16, 2017

Fixes #191.

@Nurdok Nurdok requested a review from shacharoo April 16, 2017 16:29
def foo():
a @ b
""")
parser.parse(code, 'file_path')
Copy link
Member

Choose a reason for hiding this comment

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

See if there's a way you could avoid repetition (creating the parser and giving file_path as an argument).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this merits refactoring. It's the central action in the test.

Copy link
Member

@shacharoo shacharoo Apr 17, 2017

Choose a reason for hiding this comment

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

The fact that 'file_path' appears in every test is what bothers me, not the repetition of this call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Allow me to quote myself:

The thing is, while this string appears in many tests, it is an arbitrary string, which is coincidentally the same. We gain nothing from refactoring it out into a constant, while losing readability.

@@ -271,7 +276,7 @@ class Parser(object):
def parse(self, filelike, filename):
"""Parse the given file-like object and return its Module object."""
# TODO: fix log
Copy link
Member

Choose a reason for hiding this comment

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

Remove TODO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -373,9 +380,12 @@ def parse_definitions(self, class_, all=False):
while self.current is not None:
self.log.debug("parsing definition list, current token is %r (%s)",
self.current.kind, self.current.value)
self.log.debug('got_newline: %s', self.stream.got_newline)
if all and self.current.value == '__all__':
Copy link
Member

Choose a reason for hiding this comment

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

all is a builtin function name, consider renaming this variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but not in this issue.

elif self.current.kind == tk.OP and self.current.value == '@':
elif (self.current.kind == tk.OP and
self.current.value == '@' and
self.stream.got_newline):
Copy link
Member

@shacharoo shacharoo Apr 16, 2017

Choose a reason for hiding this comment

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

I'm having trouble understanding why this works. As far as I understand, stream.got_newline only specifies if the previous token was a newline, not if the whole line was just an empty new line. Let's take your test case:

(a
@b)
@foo
def bar():
    ...

If what I said is correct, how does this condition fails to detect @b) as another decorator? I'm guessing that it's something I didn't get about the way the parser works :^)
I couldn't find anywhere a part that builds a parse tree for expressions which will ignore the contents of the parentheses.. 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the code to make this clearer, see if you get it now.

@Nurdok Nurdok merged commit 9f462f6 into PyCQA:master Apr 17, 2017
@Nurdok Nurdok deleted the feature/matrix-mul branch April 17, 2017 14:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants