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) #8720

Closed
wants to merge 52 commits into from

Conversation

hoel-bagard
Copy link
Contributor

@hoel-bagard hoel-bagard commented Nov 16, 2023

Summary

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

A first attempt at implementing E305 using physical lines was done here, and a second attempt using logical lines here. This version uses the fact that comment only lines are now counted as logical lines.

Test Plan

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

Copy link
Contributor

github-actions bot commented Nov 16, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+3911 -26 violations, +0 -0 fixes in 15 projects; 26 projects unchanged)

DisnakeDev/disnake (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ disnake/utils.py:1051:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ examples/basic_voice.py:96:9: E306 [*] Expected 1 blank line before a nested definition, found 0

RasaHQ/rasa (+13 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ data/test_classes/nlu_component_skeleton.py:11:1: E302 [*] Expected 2 blank lines, found 1
+ rasa/core/agent.py:117:9: E306 [*] Expected 1 blank line before a nested definition, found 0
+ rasa/core/channels/console.py:144:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ rasa/core/channels/console.py:218:13: E306 [*] Expected 1 blank line before a nested definition, found 0
+ rasa/core/channels/rasa_chat.py:46:9: E306 [*] Expected 1 blank line before a nested definition, found 0
+ rasa/server.py:579:13: E306 [*] Expected 1 blank line before a nested definition, found 0
+ scripts/evaluate_release_tag.py:41:1: E302 [*] Expected 2 blank lines, found 1
+ stubs/aio_pika/robust_channel.pyi:10:1: E302 [*] Expected 2 blank lines, found 1
+ stubs/sanic.pyi:23:1: E302 [*] Expected 2 blank lines, found 1
+ stubs/sanic.pyi:25:5: E301 [*] Expected 1 blank line, found 0
... 3 additional changes omitted for project

apache/airflow (+32 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ airflow/compat/functools.pyi:27:1: E302 [*] Expected 2 blank lines, found 1
+ airflow/decorators/__init__.pyi:158:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:314:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:61:1: E302 [*] Expected 2 blank lines, found 1
+ airflow/decorators/__init__.pyi:642:1: E305 [*] expected 2 blank lines after class or function definition, found (1)
+ airflow/jobs/triggerer_job_runner.py:601:13: E306 [*] Expected 1 blank line before a nested definition, found 0
+ airflow/providers/amazon/aws/hooks/s3.py:455:9: E306 [*] Expected 1 blank line before a nested definition, found 0
+ airflow/providers/amazon/aws/hooks/s3.py:476:9: E306 [*] Expected 1 blank line before a nested definition, found 0
+ airflow/providers/amazon/aws/hooks/s3.py:582:13: E306 [*] Expected 1 blank line before a nested definition, found 0
+ airflow/providers/amazon/aws/hooks/s3.py:619:9: E306 [*] Expected 1 blank line before a nested definition, found 0
... 22 additional changes omitted for project

aws/aws-sam-cli (+114 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ tests/integration/testdata/buildcmd/Provided/main.py:3:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/buildcmd/custom_root_dir/main.py:3:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/buildcmd/custom_root_dir_and_custom_makefile/custom_src_code/main.py:3:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/buildcmd/custom_root_dir_custom_makefile_and_custom_working_dir/working_dir/main.py:3:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/buildcmd/custom_working_dir_src_code/working_dir/main.py:3:1: E302 [*] Expected 2 blank lines, found 1
+ tests/integration/testdata/buildcmd/provided_src_code_without_makefile/main.py:3:1: E302 [*] Expected 2 blank lines, found 1
... 108 additional changes omitted for rule E302
+ tests/integration/testdata/start_api/lambda_authorizers/app.py:159:5: E303 [*] Too many blank lines (2)
... 107 additional changes omitted for project

bokeh/bokeh (+3208 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

+ docs/bokeh/source/conf.py:230:1: E302 [*] Expected 2 blank lines, found 1
+ examples/advanced/extensions/gears/gears.py:102:1: E305 [*] expected 2 blank lines after class or function definition, found (1)
+ examples/advanced/extensions/gears/gears.py:13:1: E302 [*] Expected 2 blank lines, found 1
+ examples/advanced/extensions/gears/gears.py:16:1: E305 [*] expected 2 blank lines after class or function definition, found (1)
+ examples/advanced/extensions/gears/gears.py:21:1: E302 [*] Expected 2 blank lines, found 1
+ examples/advanced/extensions/gears/gears.py:34:1: E302 [*] Expected 2 blank lines, found 1
+ examples/advanced/extensions/gears/gears.py:62:1: E302 [*] Expected 2 blank lines, found 1
+ examples/advanced/extensions/parallel_plot/parallel_plot.py:109:1: E305 [*] expected 2 blank lines after class or function definition, found (1)
+ examples/advanced/extensions/putting_together.py:85:1: E302 [*] Expected 2 blank lines, found 1
+ examples/advanced/extensions/putting_together.py:93:1: E305 [*] expected 2 blank lines after class or function definition, found (1)
+ examples/advanced/extensions/tool.py:74:1: E302 [*] Expected 2 blank lines, found 1
... 2390 additional changes omitted for rule E302
+ examples/advanced/extensions/tool.py:78:1: E305 [*] expected 2 blank lines after class or function definition, found (1)
+ examples/advanced/extensions/widget.py:52:1: E305 [*] expected 2 blank lines after class or function definition, found (1)
+ examples/basic/annotations/colorbar_log.py:19:1: E305 [*] expected 2 blank lines after class or function definition, found (1)
... 313 additional changes omitted for rule E305
+ examples/output/apis/autoload_static.py:34:5: E301 [*] Expected 1 blank line, found 0
+ examples/output/apis/autoload_static.py:43:5: E301 [*] Expected 1 blank line, found 0
+ release/deploy.py:55:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ src/bokeh/client/states.py:110:5: E303 [*] Too many blank lines (2)
+ src/bokeh/document/events.py:234:5: E303 [*] Too many blank lines (2)
+ src/bokeh/document/events.py:518:9: E303 [*] Too many blank lines (2)
+ src/bokeh/driving.py:114:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ src/bokeh/driving.py:170:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ src/bokeh/driving.py:189:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ src/bokeh/driving.py:91:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ src/bokeh/embed/__init__.py:54:1: E303 [*] Too many blank lines (3)
+ src/bokeh/layouts.py:323:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ src/bokeh/layouts.py:427:5: E306 [*] Expected 1 blank line before a nested definition, found 0
... 185 additional changes omitted for rule E306
+ src/bokeh/model/model.py:552:13: E301 [*] Expected 1 blank line, found 0
+ src/bokeh/model/model.py:72:5: E303 [*] Too many blank lines (2)
+ src/bokeh/models/plots.py:907:5: E301 [*] Expected 1 blank line, found 0
+ src/bokeh/models/ui/dialogs.py:58:5: E303 [*] Too many blank lines (2)
+ src/bokeh/models/widgets/tables.py:154:1: E303 [*] Too many blank lines (3)
... 36 additional changes omitted for rule E303
+ src/bokeh/server/server.py:283:5: E301 [*] Expected 1 blank line, found 0
+ src/bokeh/server/session.py:231:9: E301 [*] Expected 1 blank line, found 0
+ src/typings/selenium/webdriver/common/action_chains.pyi:10:5: E301 [*] Expected 1 blank line, found 0
... 253 additional changes omitted for rule E301
+ tests/unit/bokeh/server/test_server__server.py:469:1: E304 [*] blank lines found after function decorator
... 3172 additional changes omitted for project

commaai/openpilot (+394 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ common/api/__init__.py:10:1: E302 [*] Expected 2 blank lines, found 1
+ common/conversions.py:3:1: E302 [*] Expected 2 blank lines, found 1
+ common/ffi_wrapper.py:14:1: E302 [*] Expected 2 blank lines, found 1
+ common/ffi_wrapper.py:8:1: E302 [*] Expected 2 blank lines, found 1
+ common/file_helpers.py:74:1: E302 [*] Expected 2 blank lines, found 1
+ common/gpio.py:12:1: E302 [*] Expected 2 blank lines, found 1
... 305 additional changes omitted for rule E302
+ common/kalman/tests/test_simple_kalman.py:86:1: E305 [*] expected 2 blank lines after class or function definition, found (1)
+ common/logging_extra.py:216:1: E305 [*] expected 2 blank lines after class or function definition, found (1)
+ common/transformations/camera.py:73:1: E303 [*] Too many blank lines (3)
+ common/transformations/model.py:14:1: E305 [*] expected 2 blank lines after class or function definition, found (0)
... 384 additional changes omitted for project

freedomofpress/securedrop (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ molecule/testinfra/app/test_tor_hidden_services.py:11:1: E302 [*] Expected 2 blank lines, found 1
+ redwood/redwood/__init__.pyi:6:1: E302 [*] Expected 2 blank lines, found 1

fronzbot/blinkpy (+6 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ blinkpy/blinkpy.py:422:17: E306 [*] Expected 1 blank line before a nested definition, found 0
+ blinkpy/camera.py:415:9: E306 [*] Expected 1 blank line before a nested definition, found 0
+ blinksync/forms.py:56:5: E301 [*] Expected 1 blank line, found 0
+ blinksync/forms.py:71:5: E303 [*] Too many blank lines (2)
+ blinksync/forms.py:77:1: E302 [*] Expected 2 blank lines, found 1
+ blinksync/forms.py:9:1: E302 [*] Expected 2 blank lines, found 1

lnbits/lnbits (+27 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ lnbits/commands.py:498:9: E306 [*] Expected 1 blank line before a nested definition, found 0
+ lnbits/commands.py:512:9: E306 [*] Expected 1 blank line before a nested definition, found 0
+ lnbits/commands.py:547:9: E306 [*] Expected 1 blank line before a nested definition, found 0
+ lnbits/core/services.py:294:9: E306 [*] Expected 1 blank line before a nested definition, found 0
+ lnbits/core/services.py:325:13: E306 [*] Expected 1 blank line before a nested definition, found 0
+ lnbits/core/services.py:347:13: E306 [*] Expected 1 blank line before a nested definition, found 0
+ lnbits/core/services.py:389:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ lnbits/core/services.py:424:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ lnbits/core/services.py:488:5: E306 [*] Expected 1 blank line before a nested definition, found 0
+ lnbits/core/services.py:574:5: E306 [*] Expected 1 blank line before a nested definition, found 0
... 17 additional changes omitted for project

... Truncated remaining completed projected reports due to GitHub comment length restrictions

Changes by rule (7 rules affected)

code total + violation - violation + fix - fix
E302 2887 2887 0 0 0
E305 388 388 0 0 0
E301 270 270 0 0 0
E306 265 265 0 0 0
E303 74 74 0 0 0
PLR0917 52 26 26 0 0
E304 1 1 0 0 0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@hoel-bagard hoel-bagard marked this pull request as draft November 16, 2023 15:44
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.
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Nov 17, 2023
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.
@allisonkarlitskaya
Copy link
Contributor

With PyCQA/pycodestyle#1215 merged, the "top-levelness instead of class/def" approach now seems indisputably correct.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks for working on this (again).

I left a few comments on how I think the state could be simplified which should make it easier to reason about and review the code changes.

I still feel somewhat uncomfortable to integrate this into logical lines. Are there examples where it is important that the rule runs on logical lines? If so, then using logical lines is the right thing to do. If not, then it might be overkill and it might be easier to just copy over of what's needed from LogicalLine into a BlankLine checker.

Diagnostic::new(TooManyBlankLines(line.line.blank_lines), token.range);

let chars_to_remove = if indent_level > 0 {
line.line.preceding_blank_characters - BlankLinesConfig::METHOD
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to compute the blank lines ad-hoc? Storing them for every line seems expensive, especially because they're only needed for fixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't a way to go back to previous logical lines, right ?
Then the only option (afaik) would be to work with text and to go back until finding the end of the previous non-blank line. My understanding is that it would be slow, however if that is run only for fixes, then maybe it's fine.

@MichaReiser
Copy link
Member

I guess I need to get a better understanding when this rule is supposed to run. What I'm wondering is if it is sufficient to only test for empty lines after each Newline (which indicates a new logical line) and use Indent and Dedent to detect the end of a class / function. If that happens to be sufficient, it might be easier to implement it without using logical lines. If there are cases where this is not sufficient, than using logical lines is the right choice.

@hoel-bagard
Copy link
Contributor Author

Thank you for your review.

I don't know if using logical lines is required. I was under the impression that I could only use physical or logical lines, and with physical lines ruled out there was only logical lines left. As long as it's possible to check for indents/detents, def/async def, class and decorator tokens, I suppose it might be possible to not use logical lines.
I don't have a good idea of what the alternative would be, so it's a bit hard to say.

@MichaReiser
Copy link
Member

MichaReiser commented Dec 1, 2023

The alternative is that you create a function similar to check_logical_lines that accepts all tokens. It would be interesting to understand if doing so would require duplicating a lot of logic (in which case it isn't worth it) or if it may even be simpler because you don't need to make it fit into the logical lines structure.

Do you want to play around with it to see how it feels? Feel free to publish it as a separate (or stacked) PR. I don't want that you have to undo the work if logical lines was the right place to begin with.

See

pub(crate) fn check_logical_lines(
tokens: &[LexResult],
locator: &Locator,
stylist: &Stylist,
settings: &LinterSettings,
) -> Vec<Diagnostic> {

@hoel-bagard
Copy link
Contributor Author

Would the main goal be to not store the number of preceding blank lines/characters for each line ?

@MichaReiser
Copy link
Member

Would the main goal be to not store the number of preceding blank lines/characters for each line ?

The main goal is to understand if the rules fit into the logical lines or not. To me, it seems they don't really re-use the logic of logical lines, but it adds complexity to the logical line data structures (making it harder to understand and maintain). Meaning, the overall implementation may be more complicated than it has to. It also has the downside that running the logical line rules only (excluding blank lines) now pays for some of the blank line's runtime cost without using it. .

@hoel-bagard
Copy link
Contributor Author

hoel-bagard commented Dec 10, 2023

Ok, I'll try to make the change.

@minusf
Copy link

minusf commented Dec 13, 2023

very happy to see E303 coming, thank you all! my ocd will thank you <3

@Avasam
Copy link

Avasam commented Dec 14, 2023

As a note, autofixes for all these rules would cover E3 fixes of Autopep8

@hoel-bagard
Copy link
Contributor Author

hoel-bagard commented Dec 16, 2023

@MichaReiser I've made a version that does not use logical lines here.

Overall I feel like the added logic isn't too complex, especially since it removes some functions from the logical lines version. I still need to operate on logical lines (since pycodestyle did), so I'm still building them, but I only keep track of what I actually need.

Copy link

codspeed-hq bot commented Dec 16, 2023

CodSpeed Performance Report

Merging #8720 will not alter performance

Comparing hoel-bagard:add_blank_lines_E30_V2 (29849d5) with main (0029b4f)

Summary

✅ 30 untouched benchmarks

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

Successfully merging this pull request may close these issues.

6 participants