Fix Issue 400: Require two blank lines after toplevel def, class

[ArcTanSusan]

The original author wrote #402 to fix #400, which has been in-active since August 2015. I've rebased the commits and squashed the work into 1 commit here.

[IanLee1521]

Thanks! This looks good to me.

Could you add a note in the CHANGELOG before I merge it? Mentioning that its a change which adds a new check (E305) ?

Also, you'll need to update the error codes table in the docs to include this new error code.

[sigmavirus24]

What about changing the message to be:

E305 expected 2 blank lines after class or function definition, found %d

[jayvdb]

It would be more reusable if it was a list of all previous logical lines, and the check function finds the relevant previous lines.

[sigmavirus24]

@jayvdb why would it be more useful? I see no practical value in storing every completely non-indented logical line in a file.

[jayvdb]

@sigmavirus24 , who suggested storing non-indented logical lines? That would be silly, bordering on bizarre, as that isnt what pycodestyle currently builds and provides to checkers.

I believe it will be useful to have all of the logical lines available in order to solve other edge cases in pycodestyle (like this one) and checkers not part of the pycodestyle package. The rationale is the Zero one infinity rule.

There is a small mostly theoretical memory footprint cost of keeping the previous logical lines already created, linear with file size, but that cost is only a real serious cost if the previous logical lines are garbage collected during the processing of one file, which I havent noticed even on fairly large source files being processed on a normal system. For a bare-bones implementation, the remaining footprint cost would be the list of logical lines each stored as a tuple with an int and a pointer to a str. Additional logical line attributes (that are already currently computed) could be added to the tuple, and each logical line could be a dict instead of a tuple for a nicer interface for backwards compatibility if another attribute is added. If others generally agree that a list of previous logical lines would be good, I'd be happy to write up a new issue where the nuts and bolts can be discussed.

If we want to get this patch in and refactor later, the attribute could be called _previous_logical_toplevel here so that we don't need to think about a good name for it now, or support it later. ;-)

[sigmavirus24]

who suggested storing non-indented logical lines? That would be silly, bordering on bizarre, as that isnt what pycodestyle currently builds and provides to checkers.

This patch currently only stores the previous non-indented logical line. This is not storing the previous logical line because the Checker already does that. Currently pycodestyle is already following the Zero one infinity rule in that regard. What we're talking about in this patch is storing the previous non-indented logical line. So in the context of this patch, I understood your suggestion to be about storing all non-indented logical lines.

in order to solve other edge cases in pycodestyle

This isn't an edge case of not having all previous logical lines. This is an edge case where we need to know what the last non-indented logical line was. You're confusing the discussion on this patch.

Until we have found these other edge cases, I'm against adding the complete list of logical lines in a file, merely in an effort to keep pycodestyle as performant as it can be. (Appending to an ever growing list would work against that very effort.)

which I havent noticed even on fairly large source files being processed on a normal system.

So you've tested this and can share the tests separately?

If we want to get this patch in and refactor later

I really think this discussion is completely orthogonal to this pull request and here's why:

1. Logical lines are built from tokens and ignore indentation
2. In this pull request, we only care about logical lines that we know weren't indented (which we know through other state on the Checker object)
3. Storing all logical lines may be useful, but I don't have any concrete examples of why or when and why that list would be sufficient to replace the attribute added here and the associated state of the checker (in fact, to be truly useful, the list probably needs a lot more than the logical line strings themselves)
4. This pull request is solving the problem in the simplest way possible while still being correct (which is in the spirit of the project to date)

So please, let's curb this side discussion that is not helping @ArcTanSusan or the other reviewers.

[jayvdb]

Ok, coming at this from a different direction in the hope we can find consensus that Checker attributes for special cases are bad design.

"Top level" as a concept is ambiguous. It may change. I've left a comment on #400 about this, so discussion about the PEP can continue there.

We shouldnt have public Checker state attributes that implement specific check logic. That creates backwards compatibility problems, as the PEP does evolve over time.

If this is going to add a Checker state attribute for the most recent last logical line without any indent, we should name it accordingly. say no_indent_logical_line

[sigmavirus24]

So with the definition of "top-level" as meaning "global scope" we should be enforcing this in the following cases:

def foo(): pass


def bar(): pass


class Foo: pass


class Bar: pass


if __name__ == '__main__':
    foo()

We should also be enforcing this here:

if c_ext:
    class Foo(c_ext.Foo): pass


    class Bar(c_ext.Bar): pass


else:
    class Foo(object): pass


    class Bar(object): pass


if some_other_cond:
    def biz(): pass

Presently, the way pycodestyle works we track indentation and not scope. I think it's also clear that this pull request is an improvement (but not a complete fix) for #400.

I also support @jayvdb's suggested name of previous_unindented_logical_line, but I wonder if we should scrap this approach altogether for something that tracks whether we're in the global scope or not and uses that to generate the warning we expect to be produced.

The logic for determining if we're in a global scope (based on what tools already exist for pycodestyle) will be tricky so we'll have to test it thoroughly and comment it well to prevent it from breaking in the future. We will probably also want to use private state to update the attribute for determining our current scope. (To be clear, I don't think we need anything other than "yes we're defining a function/class in global state" and "no we're not".)

[jayvdb]

fwiw, I think we should proceed with a new private attribute (prefixed with _) so that we have fixed the extremely common case of if __name__ == '__main__': after classes and functions. It is fairly neatly isolated addition to Checker.

Then we can come up with a more complex solution for the indented definitions and all that entails (#366 would seem like a good place to track that), and back out the private attribute at the same time.

[IanLee1521]

This should actually be up in the unreleased next version changes section.

[benesch]

Thanks, @ArcTanSusan! 🍻