Special case for nested functions and classes (fixes #28)

[memeplex]

No description provided.

[sigmavirus24]

I think you can ignore appveyor

[sigmavirus24]

Ah, I didn't realize this was to prevent the test from being confused.

[sigmavirus24]

This looks 💯 👍 to me.

[memeplex]

@sigmavirus24 I hesitated to import itertools but I would like to replace the backwards slicing by islice to avoid the list copy. Maybe I should benchmark that. What do you prefer?

[sigmavirus24]

A benchmark would be good. I suspect that on large files (thousands of lines) then islice will be beneficial. It would be good to have proof. If you want a sample, try one of the openstack projects. (I think Nova has lots of nested function definitions.)

[memeplex]

Another approach may be to keep a list or deque of ancestors of the current line all the time. This is as easy as adding an ancestor to the list each time indentation increases and removing all ancestors with indentation greater than the current line each time indentation decreases. What I like of this, besides not having to (re)search for ancestors each time a checker is run, is that there would be some simple but useful representation of the current branch of the "ast" available to other checkers. This will render questions like "Am I in the else part of an if?", "Am I inside a function definition?" easy to answer.

OTOH, the current approach may be improved a bit by using a lru cache ancestors(line_number).

[IanLee1521]

Would it make sense to store the result of expand_indent(line) since you use it again in the next line?

Could be the same with line.strip() which is reused as line.lstrip() in L280.

[memeplex]

I decided to do it that way because it keeps the code shorter and the second evaluation is not very frequent.

[IanLee1521]

Fair enough, it's a minor nit anyways. :)

[memeplex]

Regarding another approach please read my last comments at the conversation tab.

[memeplex]

@sigmavirus24 @IanLee1521 Testing this I have found an unrelated bug for E305:

    elif (logical_line and not indent_level and blank_before != 2 and
          previous_unindented_logical_line.startswith(('def', 'class'))):
        yield 0, "E305 expected 2 blank lines after " \
            "class or function definition, found %d" % blank_before

will consider the following an E305:

default_x = 2
default_y = 2
default_z = 2

A match with 'def ' or 'class ' should be requested.

Do you want me to patch this here and also add a test? I could make another PR but merging with this will surely raise conflicts, since the same fragment of code is involved.

[memeplex]

Can we move forward with this (I don't want to be insistent, but #28 is open since 2011!)? Do you want me to also fix the default bug pointed out above as part of this?

[sigmavirus24]

Do you want me to also fix the default bug pointed out above as part of this?

No. Please let's keep pull requests narrowly scoped. In the event we need to revert one, it'll be easier than having to revert a myriad of changes stuffed into one.

[sigmavirus24]

So I'm fine with this PR still. I don't think @IanLee1521 wants me merging things on this repository though, so you'll have to wait for him @memeplex.

As a side note, just because an issue has been open for a long while does not implicitly make this more important than anything else that may be preventing maintainers from having the time to thoroughly review or merge this. The project is provided for free just as @IanLee1521's time and efforts are provided free of charge. You've provided an excellent contribution and I'm certain it will be merged. It isn't as if this PR has been ignored while new releases have been created. It's merely that the maintainer has not had sufficient time. Thank you for your contribution.

[sigmavirus24]

I received an email from @IanLee1521 giving me permission to merge this @memeplex. Thanks again for working on this! 🍰

[memeplex]

Thank you guys :). I will be PRing the bugfix quite soon.