Skip to content

Commit

Permalink
[pycodestyle] Respect isort settings in blank line rules (E3*) (#…
Browse files Browse the repository at this point in the history
…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
#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>
  • Loading branch information
MichaReiser and hoel-bagard authored Mar 5, 2024
1 parent d441338 commit 46ab9de
Show file tree
Hide file tree
Showing 16 changed files with 1,565 additions and 248 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# These rules test for intentional "odd" formatting. Using a formatter fixes that
[E{1,2,3}*.py]
generated_code = true
ij_formatter_enabled = false

[W*.py]
generated_code = true
ij_formatter_enabled = false
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import json



from typing import Any, Sequence


class MissingCommand(TypeError): ... # noqa: N818


class BackendProxy:
backend_module: str
backend_object: str | None
backend: Any


if __name__ == "__main__":
import abcd


abcd.foo()

def __init__(self, backend_module: str, backend_obj: str | None) -> None: ...

if TYPE_CHECKING:
import os



from typing_extensions import TypeAlias


abcd.foo()

def __call__(self, name: str, *args: Any, **kwargs: Any) -> Any:
...

if TYPE_CHECKING:
from typing_extensions import TypeAlias

def __call__2(self, name: str, *args: Any, **kwargs: Any) -> Any:
...


def _exit(self) -> None: ...


def _optional_commands(self) -> dict[str, bool]: ...


def run(argv: Sequence[str]) -> int: ...


def read_line(fd: int = 0) -> bytearray: ...


def flush() -> None: ...


from typing import Any, Sequence

class MissingCommand(TypeError): ... # noqa: N818
9 changes: 1 addition & 8 deletions crates/ruff_linter/src/checkers/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,7 @@ pub(crate) fn check_tokens(
Rule::BlankLinesAfterFunctionOrClass,
Rule::BlankLinesBeforeNestedDefinition,
]) {
let mut blank_lines_checker = BlankLinesChecker::default();
blank_lines_checker.check_lines(
tokens,
locator,
stylist,
settings.tab_size,
&mut diagnostics,
);
BlankLinesChecker::new(locator, stylist, settings).check_lines(tokens, &mut diagnostics);
}

if settings.rules.enabled(Rule::BlanketNOQA) {
Expand Down
73 changes: 70 additions & 3 deletions crates/ruff_linter/src/rules/pycodestyle/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ mod tests {

use crate::line_width::LineLength;
use crate::registry::Rule;
use crate::rules::pycodestyle;
use crate::rules::{isort, pycodestyle};
use crate::settings::types::PreviewMode;
use crate::test::test_path;
use crate::{assert_messages, settings};
Expand Down Expand Up @@ -138,14 +138,23 @@ mod tests {
Path::new("E25.py")
)]
#[test_case(Rule::MissingWhitespaceAroundParameterEquals, Path::new("E25.py"))]
fn logical(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Path::new("pycodestyle").join(path).as_path(),
&settings::LinterSettings::for_rule(rule_code),
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Rule::BlankLineBetweenMethods, Path::new("E30.py"))]
#[test_case(Rule::BlankLinesTopLevel, Path::new("E30.py"))]
#[test_case(Rule::TooManyBlankLines, Path::new("E30.py"))]
#[test_case(Rule::BlankLineAfterDecorator, Path::new("E30.py"))]
#[test_case(Rule::BlankLinesAfterFunctionOrClass, Path::new("E30.py"))]
#[test_case(Rule::BlankLinesBeforeNestedDefinition, Path::new("E30.py"))]

fn logical(rule_code: Rule, path: &Path) -> Result<()> {
fn blank_lines(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Path::new("pycodestyle").join(path).as_path(),
Expand All @@ -155,6 +164,64 @@ mod tests {
Ok(())
}

/// Tests the compatibility of the blank line top level rule and isort.
#[test_case(-1, 0)]
#[test_case(1, 1)]
#[test_case(0, 0)]
#[test_case(4, 4)]
fn blank_lines_top_level_isort_compatibility(
lines_after_imports: isize,
lines_between_types: usize,
) -> Result<()> {
let snapshot = format!(
"blank_lines_top_level_isort_compatibility-lines-after({lines_after_imports})-between({lines_between_types})"
);
let diagnostics = test_path(
Path::new("pycodestyle").join("E30_isort.py"),
&settings::LinterSettings {
isort: isort::settings::Settings {
lines_after_imports,
lines_between_types,
..isort::settings::Settings::default()
},
..settings::LinterSettings::for_rules([
Rule::BlankLinesTopLevel,
Rule::UnsortedImports,
])
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

/// Tests the compatibility of the blank line too many lines and isort.
#[test_case(-1, 0)]
#[test_case(1, 1)]
#[test_case(0, 0)]
#[test_case(4, 4)]
fn too_many_blank_lines_isort_compatibility(
lines_after_imports: isize,
lines_between_types: usize,
) -> Result<()> {
let snapshot = format!("too_many_blank_lines_isort_compatibility-lines-after({lines_after_imports})-between({lines_between_types})");
let diagnostics = test_path(
Path::new("pycodestyle").join("E30_isort.py"),
&settings::LinterSettings {
isort: isort::settings::Settings {
lines_after_imports,
lines_between_types,
..isort::settings::Settings::default()
},
..settings::LinterSettings::for_rules([
Rule::TooManyBlankLines,
Rule::UnsortedImports,
])
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test]
fn constant_literals() -> Result<()> {
let diagnostics = test_path(
Expand Down
Loading

0 comments on commit 46ab9de

Please sign in to comment.