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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/pydocstyle/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import tokenize as tk
from itertools import chain, dropwhile
from re import compile as re
from .utils import log

try:
from StringIO import StringIO
Expand Down Expand Up @@ -223,17 +224,21 @@ def __init__(self, message):


class TokenStream(object):
NEWLINES = {tk.NEWLINE, tk.INDENT, tk.DEDENT}

def __init__(self, filelike):
self._generator = tk.generate_tokens(filelike.readline)
self.current = Token(*next(self._generator, None))
self.line = self.current.start[0]
self.log = logging.getLogger()
self.log = log
self.got_newline = True

def move(self):
previous = self.current
current = self._next_from_generator()
self.current = None if current is None else Token(*current)
self.line = self.current.start[0] if self.current else self.line
self.got_newline = (previous.kind in self.NEWLINES)
return previous

def _next_from_generator(self):
Expand Down Expand Up @@ -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.

self.log = logging.getLogger()
self.log = log
self.source = filelike.readlines()
src = ''.join(self.source)
try:
Expand Down Expand Up @@ -336,6 +341,8 @@ def parse_decorators(self):
at_arguments = False

while self.current is not None:
self.log.debug("parsing decorators, current token is %r (%s)",
self.current.kind, self.current.value)
if (self.current.kind == tk.NAME and
self.current.value in ['def', 'class']):
# Done with decorators - found function or class proper
Expand Down Expand Up @@ -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.

self.parse_all()
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.

self.consume(tk.OP)
self.parse_decorators()
elif self.current.value in ['def', 'class']:
Expand Down Expand Up @@ -471,6 +481,7 @@ def parse_definition(self, class_):
assert self.current.kind != tk.INDENT
docstring = self.parse_docstring()
decorators = self._accumulated_decorators
self.log.debug("current accumulated decorators: %s", decorators)
self._accumulated_decorators = []
self.log.debug("parsing nested definitions.")
children = list(self.parse_definitions(class_))
Expand Down
43 changes: 42 additions & 1 deletion src/tests/parser_test.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
"""Parser tests."""

import six
import sys
import pytest
import textwrap
from pydocstyle.parser import Parser, Decorator, ParseError
from pydocstyle.parser import Parser, ParseError


class CodeSnippet(six.StringIO):
Expand Down Expand Up @@ -416,6 +417,46 @@ def test_raise_from():
parser.parse(code, 'file_path')


@pytest.mark.skipif(six.PY2, reason='Matrix multiplication operator is '
'invalid in Python 2.x')
def test_simple_matrix_multiplication():
"""Make sure 'a @ b' doesn't trip the parser."""
if sys.version_info.minor < 5:
return
parser = Parser()
code = CodeSnippet("""
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.



@pytest.mark.skipif(six.PY2, reason='Matrix multiplication operator is '
'invalid in Python 2.x')
def test_matrix_multiplication_with_decorators():
"""Make sure 'a @ b' doesn't trip the parser."""
if sys.version_info.minor < 5:
return
parser = Parser()
code = CodeSnippet("""
def foo():
a @ b
(a
@b)
@a
def b():
pass
""")
module = parser.parse(code, 'file_path')

outer_function, = module.children
assert outer_function.name == 'foo'

inner_function, = outer_function.children
assert len(inner_function.decorators) == 1
assert inner_function.decorators[0].name == 'a'


def test_module_publicity():
"""Test that a module that has a single leading underscore is private."""
parser = Parser()
Expand Down