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

Docstring may also be prefixed with U and R. #878

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xuhdev
Copy link

@xuhdev xuhdev commented Jul 28, 2019

No description provided.

@xuhdev xuhdev force-pushed the docstring branch 3 times, most recently from 46ffca9 to e15665b Compare July 28, 2019 07:15
@FichteFoll
Copy link
Contributor

FichteFoll commented Jul 28, 2019

Also just f. (apparently not)

pycodestyle.py Outdated Show resolved Hide resolved
@xuhdev xuhdev changed the title Docstring may also be prefixed with f, U, R, and F. Docstring may also be prefixed with U and R. Jul 28, 2019
@@ -133,7 +133,7 @@ def lru_cache(maxsize=128): # noqa as it's a fake implementation.
RAISE_COMMA_REGEX = re.compile(r'raise\s+\w+\s*,')
RERAISE_COMMA_REGEX = re.compile(r'raise\s+\w+\s*,.*,\s*\w+\s*$')
ERRORCODE_REGEX = re.compile(r'\b[A-Z]\d{3}\b')
DOCSTRING_REGEX = re.compile(r'u?r?["\']')
Copy link
Member

Choose a reason for hiding this comment

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

russian strings (ru'foo') are not valid -- could you also add a test case to demonstrate your change while you're at it -- thanks!

$ python2
Python 2.7.15+ (default, Nov 27 2018, 23:36:35) 
[GCC 7.3.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> ru'foo'
  File "<stdin>", line 1
    ru'foo'
          ^
SyntaxError: invalid syntax
>>> 
$ python3
Python 3.6.8 (default, Jan 14 2019, 11:02:34) 
[GCC 8.0.1 20180414 (experimental) [trunk revision 259383]] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> ru'foo'
  File "<stdin>", line 1
    ru'foo'
          ^
SyntaxError: invalid syntax

Copy link
Member

Choose a reason for hiding this comment

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

@xuhdev I'm looking at the PR diff here and am not seeing a test or change that matches what @asottile asked for

@xuhdev
Copy link
Author

xuhdev commented Aug 12, 2019

I updated the PR. I'm not sure how to add relevant test though; pycodestyle does not appear to check specifically for string prefixes.

@asottile
Copy link
Member

I updated the PR. I'm not sure how to add relevant test though; pycodestyle does not appear to check specifically for string prefixes.

I'm not sure what this PR fixes then -- did you have a particular case where this fixed something?

I was able to debug what this is intended to protect against (I think?)

--- a/pycodestyle.py
+++ b/pycodestyle.py
@@ -374,6 +374,9 @@ def blank_lines(logical_line, blank_lines, indent_level, line_number,
                     previous_indent_level < indent_level or
                     DOCSTRING_REGEX.match(previous_logical)
                     ):
+
+                if not DOCSTRING_REGEX.match(previous_logical):
+                    breakpoint()
                 ancestor_level = indent_level
                 nested = False
                 # Search backwards for a def ancestor or tree root

this is hit for the following testcase from the testsuite:

class X:

    def a():
        pass
    def b():
        pass

Adjusting this slightly:

class X:

    def a():
        """docstring"""
    def b():
        pass

that passes with no errors:

$ python3.7 -m pycodestyle t.py
$

But, if we adjust it:

class X:

    def a():
        U"""docstring"""
    def b():
        pass
$ python3.7 -m pycodestyle t.py
t.py:5:5: E301 expected 1 blank line, found 0

You can probably use this difference to write a testcase I imagine?

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.

None yet

4 participants