Skip to content
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

[pycodestyle] Add blank line(s) rules (E301, E302, E303, E304, E305, E306) #9266

Merged
merged 128 commits into from Feb 8, 2024

Conversation

hoel-bagard
Copy link
Contributor

Summary

This PR is part of #2402, it adds the E301, E302, E303, E304, E305, E306 error rules along with their fixes.

This PR is based on #8720, but does not use logical lines and instead works directly on tokens.

Test Plan

The test fixture uses the one from pycodestyle with a some added tests.

hoel-bagard and others added 30 commits November 10, 2023 00:36
decorator -> comment -> decorator was causing a false positive.
Fix false positive where a method following an if (or other indentation
inducing keyword) would trigger E301.
This also move the trigger on an async def from the def to the async.
Make the comment stick to the following line instead of the preceding
one.
Comment on lines 30 to 35
/// Checks for missing blank lines between methods of a class.
///
/// ## Why is this bad?
/// PEP 8 recommends the use of blank lines as follows:
/// - Two blank lines are expected between functions and classes
/// - One blank line is expected between methods of a class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we mention the recommended spacing between functions and classes if this rule is just for methods of a class? It might make sense to just say "PEP 8 recommends exactly one blank line between methods of a class". We should also not specify "missing" if this will also trigger for "too many" blank lines, instead we should say "Checks for the correct number of blank lines between methods of a class.

We could move all of this into "What it does" and have "Why is this bad" focus on consistency with PEP 8? I don't have strong feelings about that though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we mention the recommended spacing between functions and classes if this rule is just for methods of a class? It might make sense to just say "PEP 8 recommends exactly one blank line between methods of a class".

Done in 3d5cc84

We should also not specify "missing" if this will also trigger for "too many" blank lines, instead we should say "Checks for the correct number of blank lines between methods of a class.

This rule does not trigger for too many blank lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could move all of this into "What it does" and have "Why is this bad" focus on consistency with PEP 8? I don't have strong feelings about that though.

I don't have strong feelings about this either, I can try to change it if you want.

@MichaReiser MichaReiser enabled auto-merge (squash) February 8, 2024 18:29
@MichaReiser MichaReiser merged commit 9027169 into astral-sh:main Feb 8, 2024
16 checks passed
@akx
Copy link
Contributor

akx commented Feb 9, 2024

Merged?! Yay! Thanks for the hard work @hoel-bagard!

@zanieb
Copy link
Member

zanieb commented Feb 9, 2024

Thanks for addressing my feedback!

@spaceone
Copy link
Contributor

I upgraded to ruff 0.2.2, where this is included. But it's still hidden behind some compiler flag? I can't test it without compiling ruff on my own?

@hoel-bagard
Copy link
Contributor Author

@spaceone You need to enable the preview mode, see here.

@Sam1320
Copy link

Sam1320 commented Feb 21, 2024

Great work @hoel-bagard ! I come from this SO comment I followed all the previous PRs related to this without much hope. Amazing that I would have been blocked if I checked this only 2 weeks ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants