[`pycodestyle`] Respect `isort` settings in blank line rules (`E3*`) by MichaReiser · Pull Request #10096 · astral-sh/ruff

MichaReiser · 2024-02-23T13:20:08Z

Summary

This PR changes the E3* rules to respect the isort lines-after-imports and lines-between-types settings. Specifically, the following rules required changing

TooManyBlannkLines : Respects both settings.
BlankLinesTopLevel: Respects lines-after-imports. Doesn't need to respect lines-between-types because it only applies to classes and functions

The downside of this approach is that isort and the blank line rules emit a diagnostic when there are too many blank lines. The fixes aren't identical, the blank line is less opinionated, but blank lines accepts the fix of isort.

Outdated approach

Fixes https://github.com//issues/10077#issuecomment-1961266981

This PR changes the blank line rules to not enforce the number of blank lines after imports (top-level) if isort is enabled and leave it to isort to enforce the right number of lines (depends on the isort.lines-after-imports and isort.lines-between-types settings).

The reason to give isort precedence over the blank line rules is that they are configurable. Users that always want to blank lines after imports can use isort.lines-after-imports=2 to enforce that (specifically for imports).

This PR does not fix the incompatibility with the formatter in pyi files that only uses 0 to 1 blank lines. I'll address this separately.

Review

The first commit is a small refactor that simplified implementing the fix (and makes it easier to reason about what's mutable and what's not).

Test Plan

I added a new test and verified that it fails with an error that the fix never converges. I verified the snapshot output after implementing the fix.

MichaReiser · 2024-02-23T13:22:29Z

-                stylist,
-                diagnostics,
-            );
+            state.class_status.update(&logical_line);


The state update logic used to be inside of check_line. Moving the state handling out has the benefit that we can skip calling check_line and it also separates the state update logic from the actual checking logic which I find easier to understand.

github-actions · 2024-02-23T13:41:00Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+23 -6 violations, +0 -0 fixes in 3 projects; 40 projects unchanged)

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

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/decorators/__init__.pyi:102:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:105:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:158:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:191:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:203:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:257:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:294:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:315:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:465:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:633:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:668:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:688:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:720:5: E301 [*] Expected 1 blank line, found 0
+ airflow/decorators/__init__.pyi:89:5: E301 [*] Expected 1 blank line, found 0

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

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ src/bokeh/resources.py:256:1: E302 [*] Expected 2 blank lines, found 1
+ tests/support/plugins/managed_server_loop.py:59:1: E302 [*] Expected 2 blank lines, found 1
+ tests/support/plugins/project.py:134:1: E302 [*] Expected 2 blank lines, found 1

pandas-dev/pandas (+6 -6 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- pandas/_libs/algos.pyi:100:5: PLR0917 Too many positional arguments (8/5)
+ pandas/_libs/algos.pyi:101:5: PLR0917 Too many positional arguments (8/5)
- pandas/_libs/algos.pyi:110:5: PLR0917 Too many positional arguments (7/5)
+ pandas/_libs/algos.pyi:111:5: PLR0917 Too many positional arguments (7/5)
- pandas/_libs/tslibs/offsets.pyi:271:9: PLR0917 Too many positional arguments (6/5)
+ pandas/_libs/tslibs/offsets.pyi:279:9: PLR0917 Too many positional arguments (6/5)
- pandas/_libs/tslibs/offsets.pyi:285:9: PLR0917 Too many positional arguments (6/5)
+ pandas/_libs/tslibs/offsets.pyi:294:9: PLR0917 Too many positional arguments (6/5)
- pandas/_libs/tslibs/offsets.pyi:297:9: PLR0917 Too many positional arguments (6/5)
+ pandas/_libs/tslibs/offsets.pyi:306:9: PLR0917 Too many positional arguments (6/5)
- pandas/_libs/tslibs/offsets.pyi:309:9: PLR0917 Too many positional arguments (8/5)
+ pandas/_libs/tslibs/offsets.pyi:318:9: PLR0917 Too many positional arguments (8/5)

Changes by rule (3 rules affected)

code	total	+ violation	- violation
E301	14	14	0
PLR0917	12	6	6
E302	3	3	0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

MichaReiser · 2024-02-23T13:52:47Z

Hmm, it's slightly more complicated than that...

if TYPE_CHECKING:
    from typing_extensions import TypeAlias

def model_link(fullname: str) -> str:
    # (double) escaped space at the end is to appease Sphinx
    # https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#gotchas
    return f":class:`~{fullname}`\\ "

It doesn't handle the above correctly (should raise an error) because it assumes that it is after an import.

charliermarsh · 2024-02-26T21:13:16Z

I don't know that I have better suggestions, but I'll admit that I'm nervous to make the rule contingent on whether another rule is enabled, since it means ruff check --select E and ruff check --select E --select I will show discontinuous diagnostics, which can make for a really confusing debugging experience.

MichaReiser · 2024-02-26T21:15:27Z

Hmm that's a good point. The alternative is to never touch blank lines after imports and direct users towards isort? Although that's a bit overkill if all you want is 2 blank lines

MichaReiser · 2024-02-27T12:50:56Z

So the options I see are

Remove the blank line rules
Never enforce blank lines after or between imports (forcing users to use isort)
Disable blank lines when isort is activated (what we do for other incompatible rules)
Use isort.lines_after_imports or isort.lines_between_types after imports or between imports in the blank line rule (A bit weird that it respects options from another rule)

hoel-bagard · 2024-02-27T13:13:23Z

Use isort.lines_after_imports or isort.lines_between_types after imports or between imports in the blank line rule (A bit weird that it respects options from another rule)

I was thinking about that one. As you said it's a bit weird to use options from another rule, but that allows using both rule sets.
Also, this weirdness would likely not be visible to a user (it seems unlikely to me that someone would specify the isort options but not select isort), so it would be a matter of handling the added complexity in the blank rules in a way that is maintainable.

Never enforce blank lines after or between imports (forcing users to use isort)

This option is seems more straightforward to implement, document and maintain.

MichaReiser · 2024-03-01T16:24:22Z

The ecosystem changes look okay to me. It's not the main change but i must have fixed those rules by setting self.follows to Other when the indent decreased.

MichaReiser · 2024-03-01T16:26:29Z

@hoel-bagard would you mind taking a look at the changes? I think you know the rule the best.

hoel-bagard

The changes look good to me, I just had two questions about parts that weren't clear to me.

Also, not important but some ifs were over indented so I made a PR to fix that.

Some ifs are over-indented in #10096 as a result of refactoring, this PR simply fixes that.

sciyoshi · 2024-03-04T19:32:55Z

Just to confirm, this change won't fix the issue in #10077, correct? I'm testing things out with this branch but still see the infinite loop when running on .pyi files.

MichaReiser · 2024-03-05T09:26:20Z

Just to confirm, this change won't fix the issue in #10077, correct? I'm testing things out with this branch but still see the infinite loop when running on .pyi files.

Nice find. It seems specific to a class coming after imports when using lines-after-imports=1 Let me revisit the tests to see why this case isn't covered.

Some ifs are over-indented in #10096 as a result of refactoring, this PR simply fixes that.

MichaReiser · 2024-03-05T09:45:22Z

Just to confirm, this change won't fix the issue in #10077, correct? I'm testing things out with this branch but still see the infinite loop when running on .pyi files.

Okay, the issue is specific to pyi files and is fixed by #10098

MichaReiser · 2024-03-05T09:58:43Z

@hoel-bagard thank you for your review and fixing the indentation.

…stral-sh#10096) ## Summary This PR changes the `E3*` rules to respect the `isort` `lines-after-imports` and `lines-between-types` settings. Specifically, the following rules required changing * `TooManyBlannkLines` : Respects both settings. * `BlankLinesTopLevel`: Respects `lines-after-imports`. Doesn't need to respect `lines-between-types` because it only applies to classes and functions The downside of this approach is that `isort` and the blank line rules emit a diagnostic when there are too many blank lines. The fixes aren't identical, the blank line is less opinionated, but blank lines accepts the fix of `isort`. <details> <summary>Outdated approach</summary> Fixes astral-sh#10077 (comment) This PR changes the blank line rules to not enforce the number of blank lines after imports (top-level) if isort is enabled and leave it to isort to enforce the right number of lines (depends on the `isort.lines-after-imports` and `isort.lines-between-types` settings). The reason to give `isort` precedence over the blank line rules is that they are configurable. Users that always want to blank lines after imports can use `isort.lines-after-imports=2` to enforce that (specifically for imports). This PR does not fix the incompatibility with the formatter in pyi files that only uses 0 to 1 blank lines. I'll address this separately. </details> ## Review The first commit is a small refactor that simplified implementing the fix (and makes it easier to reason about what's mutable and what's not). ## Test Plan I added a new test and verified that it fails with an error that the fix never converges. I verified the snapshot output after implementing the fix. --------- Co-authored-by: Hoël Bagard <34478245+hoel-bagard@users.noreply.github.com>

MichaReiser added bug Something isn't working preview Related to preview mode features labels Feb 23, 2024

MichaReiser force-pushed the blank-lines-isort branch from 0d05d48 to c43a07e Compare February 23, 2024 13:21

MichaReiser commented Feb 23, 2024

View reviewed changes

MichaReiser requested a review from charliermarsh February 23, 2024 13:22

MichaReiser marked this pull request as draft February 23, 2024 13:51

MichaReiser marked this pull request as ready for review February 23, 2024 14:07

MichaReiser mentioned this pull request Feb 27, 2024

Fix E301 not triggering on decorated methods. #10117

Merged

MichaReiser force-pushed the blank-lines-isort branch from 8feb4fa to 94d04a6 Compare March 1, 2024 09:25

MichaReiser changed the title ~~Disable Blank line rules for top level statements after imports~~ [pycodestyle] Respect isort settings in blank line rules (E3*) Mar 1, 2024

MichaReiser force-pushed the blank-lines-isort branch from 2fcdfbb to 066cc98 Compare March 1, 2024 15:53

MichaReiser requested review from charliermarsh and removed request for charliermarsh March 1, 2024 16:24

hoel-bagard reviewed Mar 3, 2024

View reviewed changes

Comment thread crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs Outdated

Comment thread crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs Outdated

hoel-bagard reviewed Mar 3, 2024

View reviewed changes

Comment thread crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs

hoel-bagard mentioned this pull request Mar 3, 2024

[pycodestyle] Fix indentation in #10096 #10208

Merged

MichaReiser pushed a commit that referenced this pull request Mar 3, 2024

[pycodestyle] Fix indentation in #10096 (#10208)

425e2d1

Some ifs are over-indented in #10096 as a result of refactoring, this PR simply fixes that.

charliermarsh reviewed Mar 4, 2024

View reviewed changes

Comment thread crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs Outdated

charliermarsh approved these changes Mar 4, 2024

View reviewed changes

Factor out state from BlankLinesChecker

123b68e

MichaReiser and others added 6 commits March 5, 2024 10:26

Disable blank line rules after top level import statements

fefac31

Test blank lines after nested imports

267697d

Implement isort compatibility

f23c2fa

Remove expected from TooManyBlankLines

2108ae5

Fix Typo

7b76751

[pycodestyle] Fix indentation in #10096 (#10208)

2d3537d

Some ifs are over-indented in #10096 as a result of refactoring, this PR simply fixes that.

Add example with too many blank lines after imports

1021bbf

Code review feedback

7bd7e71

MichaReiser force-pushed the blank-lines-isort branch from 425e2d1 to 7bd7e71 Compare March 5, 2024 10:02

MichaReiser enabled auto-merge (squash) March 5, 2024 10:04

MichaReiser merged commit 46ab9de into main Mar 5, 2024

MichaReiser deleted the blank-lines-isort branch March 5, 2024 10:09

BrewTestBot mentioned this pull request Mar 7, 2024

ruff 0.3.1 Homebrew/homebrew-core#165309

Merged

johnslavik mentioned this pull request Mar 15, 2024

Bump ruff-pre-commit to v0.3.2 python-poetry/cleo#412

Merged

Conversation

MichaReiser commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review

Test Plan

Uh oh!

MichaReiser Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

MichaReiser commented Feb 23, 2024

Uh oh!

charliermarsh commented Feb 26, 2024

Uh oh!

MichaReiser commented Feb 26, 2024

Uh oh!

MichaReiser commented Feb 27, 2024

Uh oh!

hoel-bagard commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoel-bagard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sciyoshi commented Mar 4, 2024

Uh oh!

MichaReiser commented Mar 5, 2024

Uh oh!

MichaReiser commented Mar 5, 2024

Uh oh!

MichaReiser commented Mar 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MichaReiser commented Feb 23, 2024 •

edited

Loading

github-actions Bot commented Feb 23, 2024 •

edited

Loading

`ruff-ecosystem` results

hoel-bagard commented Feb 27, 2024 •

edited

Loading

MichaReiser commented Mar 1, 2024 •

edited

Loading

MichaReiser commented Mar 1, 2024 •

edited

Loading

hoel-bagard left a comment •

edited

Loading