[pycodestyle] Fix E301 to only trigger for functions immediately within a class
#19768
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| E301 | 151 | 0 | 151 | 0 | 0 |
|
Nice, -148 violations! |
ntBre
added
preview
| && !(state.follows.is_any_def() && line.last_token != TokenKind::Colon) | ||
| && !state.follows.follows_def_with_dummy_body() | ||
| && matches!(state.class_status, Status::Inside(_)) | ||
| // Only apply to functions that are "immediately within" a class (not nested within other functions/blocks) |
There was a problem hiding this comment.
Can we add the reasoning why we only trigger for functions directly within a class?
What about
class Foo:
if True:
def test(): passThere was a problem hiding this comment.
I added in some additional details; let me know if it's too verbose—there may be a simpler way to put it.
There was a problem hiding this comment.
I think we need something more like this example or one of the other checks on main will short-circuit:
class Foo:
if True:
print("conditional")
def test():
passbut this version is also flagged by pycodestyle (and flake8), so I think we'll want to test that it still is:
$ pycodestyle try.py --select E301 --show-source
try.py:4:9: E301 expected 1 blank line, found 0
def test():
^I was hopeful that we could detect this a bit more robustly than checking indentation anyway.
| E30.py:993:9: E306 [*] Expected 1 blank line before a nested definition, found 0 | ||
| | | ||
| 991 | if True: | ||
| 992 | print("conditional") | ||
| 993 | def test(): | ||
| | ^^^ E306 | ||
| 994 | pass | ||
| 995 | # end | ||
| | | ||
| = help: Add missing blank line |
There was a problem hiding this comment.
Based on my comment with the pycodestyle results, this is supposed to be getting an E301, but it's still only getting an E306.
I would really prefer if we could handle these checks by updating the state tracking in this function rather than trying to compare the indentation. Have you explored that at all?
There was a problem hiding this comment.
I haven't thought of that, thank you!
|
Thanks. Could you resolve the merge conflicts? They should just be from the new diff output format. It also still looks like the |
|
Should be all set, let me know! |
|
Can this PR get a review please? It's the last thing blocking the adoption of ruff for me: GlasgowEmbedded/glasgow#993 |
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
I had one question about the E306 code and a suggestion for one more test case, but this looks like a nice fix. Thank you!
| } | ||
| } | ||
|
|
||
| if line.preceding_blank_lines == 0 |
There was a problem hiding this comment.
Do we need to move this code now? I tested locally after putting it back, and it seems fine down here still.
|
I think you may have reverted the wrong piece of code. The ecosystem check is now showing -160 E306 violations instead of -151 E301 violations. |
|
Hm, maybe I am confused—Could you point me to what part we were looking to revert? Feel free to commit from your side if it's easier, I know this has been a long PR 🙂 |
|
I'll push a commit! I may not have phrased it well, but I just wanted to move the E306 code back to where it was originally (a standalone |
|
This is what I had in mind. I'll double-check the ecosystem report to make sure it's actually equivalent. The |
|
Thank you! When can I expect the fix to be available in a release (really, I just want it to use with pre-commit.ci)? |
|
Our weekly releases are usually on Thursday, so it should be out in ~2 days! |
|
Thanks! |
4 participants
Summary
Fixes #19752