Spurious E301 for nested function definitions #28

Closed
walkerh opened this Issue Mar 17, 2011 · 18 comments

Comments

Projects
None yet

walkerh commented Mar 17, 2011

Simple nested functions without preceeding blank line are not a violation of PEP 8. It is up to the human to decide readability in this case.

pep8-0.6.1 will emit errors in cases where an extra blank line is not required or appropriate.

Example code, pep8_bug.py:

"""Demonstrate bug"""


def safe_accessors(dictionary=False):
    """simplified from complex test code..."""
    if dictionary:
        _setattr, _getattr, _delattr = setitem, getitem, delitem
        def _hasattr(_dict, value):
            return value in _dict
    else:
        _setattr, _getattr, _delattr, _hasattr = (
            setattr, getattr, delattr, hasattr
        )
    return _setattr, _getattr, _delattr, _hasattr

pep8 output:

pep8_bug.py:9:9: E301 expected 1 blank line, found 0

The correct output is to be silent (or at most emit a silence-able warning).

Reference: "PEP 8" / "Code lay-out" / "Blank Lines"

Contributor

JensRantil commented Jul 27, 2012

I am too experiencing this with pep8 1.3.3.

Contributor

JensRantil commented Jul 27, 2012

Without looking at the code, I guess the secret would be to look at the scope of the defined function. If it is in the scope of another def, no error or warning should be emitted.

@JensRantil JensRantil added a commit to JensRantil/pep8 that referenced this issue Jul 27, 2012

@JensRantil JensRantil Minimal test case for issue #28
Currently there are false negatives when nesting python functions.
82c3a9e

@attilaolah attilaolah added a commit to attilaolah/pep8 that referenced this issue Jul 12, 2013

@attilaolah attilaolah a simple fix for nested functions (fixes #28) f0d4740

attilaolah referenced this issue Jul 12, 2013

Closed

Fix #28 #219

I've attached a pull request that fixes this and makes the test added in 82c3a9e pass.

I'm not sure though what to do with classes that are nested inside functions, e.g.

def foo():
    class Test(object):
        def method_1(self):
            pass
        def method_2(self):
            pass

Should method_1 and method_2 be separated by blank lines because they are methods, even though they are define in a closure? (If yes, then I need modifications to my pull request.)

I'd like to cover much broader set of cases. What about using a whitelist instead of blacklist? We can assume that every class definition which is indented and every function definition which is indented more than 2 levels are nested definitions. This covers all cases except for a function nested in a function. This case can be covered by a variation of @attilaolah solution -- test if there is any function definition before, but stop when you find a class definition.

What about my PR jcrocholl#232?

@chrismedrela chrismedrela pushed a commit to chrismedrela/pep8 that referenced this issue Sep 29, 2013

Christopher Medrela Fixed #28 -- relaxed E301 for nested definitions e5c0576
Contributor

gward commented Nov 24, 2013

I agree with @chrismedrela: nested classes should be considered similarly to nested functions, on the grounds that PEP 8 says what to do about top-level classes and functions, and says nothing about nested functions or classes. If pep8 is going to be neutral about nested functions, then it should be equally neutral about nested classes.

Contributor

florentx commented Mar 22, 2014

PEP 8 says:

Method definitions inside a class are separated by a single blank line.

I would prefer to keep the checks for these nested classes with some new error code, which could be ignored in the configuration.

For example:

E311 expected 1 blank line, found 0 (nested definition)

scopatz referenced this issue in pyne/pyne Dec 8, 2014

Merged

PyNE in a State of Decay #614

IanLee1521 self-assigned this Dec 30, 2014

Contributor

memeplex commented Jul 20, 2015

I use nested functions and the current behavior requires (to conform) arbitrary empty lines that are often very misleading about the logic divisions inside a function.

If we can reach and agreement about which is the desired new behavior here, I will gladly implement a fix for this (or fix PR #232 conflicts).

My proposal is to keep the current E301 definition except for nested functions/classes definitions. With nested functions/classes I mean functions/classes that are local to another function, not functions/classes nested inside a class definition.

What do you think?

azag0 commented Sep 30, 2015

A temporary dirty fix would be for E301 to be included in the noqa-ignorable warnings. I don't think nested functions are that frequent and having one # noqa comment is still much more acceptable to me than having a blank line that does not make any sense.

Owner

sigmavirus24 commented Sep 30, 2015

So I just re-read some of this discussion, and I'd like to point out one statement that has gone unchallenged and is wrong on its face:

We can assume that every class definition which is indented and every function definition which is indented more than 2 levels are nested definitions.

I've maintained and contributed to several projects that use the concept of optional drivers (or adapters, or whatever you want to call it) where something is defined like so:

try:
    import module
except ImportError
    module = None


if module is None:
    class ModuleDriver:
         def some_method(self, param, param):
              # etc.

else:
    class ModuleDriver:
         def some_method(self, param, param):

This is a top-level definition. It is not a nested definition. It is a conditional definition. Anything that attempts to fix this should be mindful of this. Also keep in mind that further guesses at what indentation level "must" mean semantically can be further disproven by other nested conditionals based on definition.

Contributor

memeplex commented Oct 2, 2015

@azag0 I didn't know about any noqa-ignorable list. Is that for flake8 or for pep8?

azag0 commented Oct 2, 2015

Well, I don't know whether there is an explicit list somewhere, but I was referring to #180

Owner

sigmavirus24 commented Oct 2, 2015

Each check individually chooses to observe NOQA or not.

warsaw commented Apr 1, 2016

I just ran across this issue. My own quasi-authoritative take on this is that nested functions and classes (and the methods within those classes) do not necessarily conform to the same blank line rules as top-level functions and classes.

Nested functions and classes are a bit of a special case, they are infrequent, and typically added for convenience rather than public functionality. E.g. you might define a sort function inline, or you might have a little mock class inside of a test. Since these aren't public, module global names, succinctness is acceptable and artificial blank lines can disrupt the flow. It's also entirely acceptable to use comments to visually separate such nested functions and classes from the surrounding code.

Ultimately, I think E301 isn't appropriate for nested functions and classes.

Contributor

memeplex commented Jun 28, 2016

Well I'm about to write a patch for myself, and maybe for you. So I would like to know: if I adapt attilaolah/pep8@f0d4740 so that it throws a new error code as requested by @florentx, would it be good enough for you? This new error should be enabled or disabled by default?

Contributor

memeplex commented Jun 28, 2016

Well, I see attilaolah/pep8@f0d4740 has many issues. I will try to fix the nested-in-function detection logic.

Contributor

memeplex commented Jun 29, 2016 edited

Please, tell me what do you think while this is still fresh in my mind. Btw, do you think it could be handy for other checkers if I factor out an ancestor iterator from this code?

Contributor

memeplex commented Aug 4, 2016

Hi there, is anything else I can do to get my patch pulled in? It was reviewed favourably, a further ok test was requested but I explained why I think it doesn't make much sense here. I have also found an unrelated bug in the affected function (see the pr discussion). Please let me know if something is missing and I will do it asap.

@sigmavirus24 sigmavirus24 added a commit that referenced this issue Aug 11, 2016

@sigmavirus24 sigmavirus24 Merge pull request #555 from memeplex/master
Special case for nested functions and classes (fixes #28)
39e5718
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment