-
Notifications
You must be signed in to change notification settings - Fork 750
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
Require two blank lines after toplevel def/class; issue #400 #402
Conversation
@@ -268,6 +270,9 @@ def blank_lines(logical_line, blank_lines, indent_level, line_number, | |||
yield 0, "E301 expected 1 blank line, found 0" | |||
elif blank_before != 2: | |||
yield 0, "E302 expected 2 blank lines, found %d" % blank_before | |||
elif (logical_line and not indent_level and blank_before != 2 and | |||
previous_logical_toplevel.startswith(('def', 'class'))): | |||
yield 0, "E305 expected 2 blank lines before, found %d" % blank_before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that expect 2 blank lines after the def
or class
? Sure it's checking for the next logical line if before it there are two lines but only because the block above was a def
or class
.
Maybe also mention that it expects two lines after a method-level function or class (I know E302 doesn't mention that too but that may be improved too?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm? I'm pretty sure the condition is right: it's looking for a non-indented line that's preceded by more or less than two blank lines where the preceding non-blank non-indented line was a function or class definition.
It doesn't actually expect two lines after a method-level function or class, since those are only supposed to have one separating blank line according to PEP 8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition is right but I find the message confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, agreed! I'll defer that to another diff, though.
This hasn't been worked on since August 2015 last year. For PyCon Sprints 2016, I've fixed the conflicts in this new branch and rebase-squashed both commits into 1 commit: #536 |
Here's my attempt to fix issue #400. Not sure what the backwards compatibility policy on this'll be, so I left it as enabled by default.