Skip to content

Commit

Permalink
[pycodestyle]: Make blank lines in typing stub files optional (`E3*…
Browse files Browse the repository at this point in the history
…`) (#10098)

## Summary

Fixes #10039

The [recommendation for typing stub
files](https://typing.readthedocs.io/en/latest/source/stubs.html#blank-lines)
is to use **one** blank line to group related definitions and
otherwise omit blank lines. 

The newly added blank line rules (`E3*`) didn't account for typing stub
files and enforced two empty lines at the top level and one empty line
otherwise, making it impossible to group related definitions.

This PR implements the `E3*` rules to:

* Not enforce blank lines. The use of blank lines in typing definitions
is entirely up to the user.
* Allow at most one empty line, including between top level statements. 

## Test Plan

Added unit tests (It may look odd that many snapshots are empty but the
point is that the rule should no longer emit diagnostics)
  • Loading branch information
MichaReiser authored Mar 5, 2024
1 parent 46ab9de commit af6ea2f
Show file tree
Hide file tree
Showing 12 changed files with 577 additions and 12 deletions.
50 changes: 50 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pycodestyle/E30.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import json

from typing import Any, Sequence

class MissingCommand(TypeError): ...
class AnoherClass: ...

def a(): ...

@overload
def a(arg: int): ...

@overload
def a(arg: int, name: str): ...


def grouped1(): ...
def grouped2(): ...
def grouped3( ): ...


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

def grouped1(): ...
def grouped2(): ...
def grouped3( ): ...
@decorated

def with_blank_line(): ...


def ungrouped(): ...
a = "test"

def function_def():
pass
b = "test"


def outer():
def inner():
pass
def inner2():
pass

class Foo: ...
class Bar: ...
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
3 changes: 2 additions & 1 deletion crates/ruff_linter/src/checkers/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ pub(crate) fn check_tokens(
Rule::BlankLinesAfterFunctionOrClass,
Rule::BlankLinesBeforeNestedDefinition,
]) {
BlankLinesChecker::new(locator, stylist, settings).check_lines(tokens, &mut diagnostics);
BlankLinesChecker::new(locator, stylist, settings, source_type)
.check_lines(tokens, &mut diagnostics);
}

if settings.rules.enabled(Rule::BlanketNOQA) {
Expand Down
32 changes: 32 additions & 0 deletions crates/ruff_linter/src/rules/pycodestyle/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,38 @@ mod tests {
Ok(())
}

#[test_case(Rule::BlankLineBetweenMethods)]
#[test_case(Rule::BlankLinesTopLevel)]
#[test_case(Rule::TooManyBlankLines)]
#[test_case(Rule::BlankLineAfterDecorator)]
#[test_case(Rule::BlankLinesAfterFunctionOrClass)]
#[test_case(Rule::BlankLinesBeforeNestedDefinition)]
fn blank_lines_typing_stub(rule_code: Rule) -> Result<()> {
let snapshot = format!("blank_lines_{}_typing_stub", rule_code.noqa_code());
let diagnostics = test_path(
Path::new("pycodestyle").join("E30.pyi"),
&settings::LinterSettings::for_rule(rule_code),
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test]
fn blank_lines_typing_stub_isort() -> Result<()> {
let diagnostics = test_path(
Path::new("pycodestyle").join("E30_isort.pyi"),
&settings::LinterSettings {
..settings::LinterSettings::for_rules([
Rule::TooManyBlankLines,
Rule::BlankLinesTopLevel,
Rule::UnsortedImports,
])
},
)?;
assert_messages!(diagnostics);
Ok(())
}

#[test]
fn constant_literals() -> Result<()> {
let diagnostics = test_path(
Expand Down
63 changes: 52 additions & 11 deletions crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use ruff_diagnostics::Diagnostic;
use ruff_diagnostics::Edit;
use ruff_diagnostics::Fix;
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::PySourceType;
use ruff_python_codegen::Stylist;
use ruff_python_parser::lexer::LexResult;
use ruff_python_parser::lexer::LexicalError;
Expand Down Expand Up @@ -51,9 +52,14 @@ const BLANK_LINES_NESTED_LEVEL: u32 = 1;
/// pass
/// ```
///
/// ## Typing stub files (`.pyi`)
/// The typing style guide recommends to not use blank lines between methods except to group
/// them. That's why this rule is not enabled in typing stub files.
///
/// ## References
/// - [PEP 8](https://peps.python.org/pep-0008/#blank-lines)
/// - [Flake 8 rule](https://www.flake8rules.com/rules/E301.html)
/// - [Typing Style Guide](https://typing.readthedocs.io/en/latest/source/stubs.html#blank-lines)
#[violation]
pub struct BlankLineBetweenMethods;

Expand Down Expand Up @@ -96,9 +102,14 @@ impl AlwaysFixableViolation for BlankLineBetweenMethods {
/// pass
/// ```
///
/// ## Typing stub files (`.pyi`)
/// The typing style guide recommends to not use blank lines between classes and functions except to group
/// them. That's why this rule is not enabled in typing stub files.
///
/// ## References
/// - [PEP 8](https://peps.python.org/pep-0008/#blank-lines)
/// - [Flake 8 rule](https://www.flake8rules.com/rules/E302.html)
/// - [Typing Style Guide](https://typing.readthedocs.io/en/latest/source/stubs.html#blank-lines)
#[violation]
pub struct BlankLinesTopLevel {
actual_blank_lines: u32,
Expand Down Expand Up @@ -150,13 +161,17 @@ impl AlwaysFixableViolation for BlankLinesTopLevel {
/// pass
/// ```
///
/// ## Typing stub files (`.pyi`)
/// The rule allows at most one blank line in typing stub files in accordance to the typing style guide recommendation.
///
/// Note: The rule respects the following `isort` settings when determining the maximum number of blank lines allowed between two statements:
/// * [`lint.isort.lines-after-imports`]: For top-level statements directly following an import statement.
/// * [`lint.isort.lines-between-types`]: For `import` statements directly following a `from ... import ...` statement or vice versa.
///
/// ## References
/// - [PEP 8](https://peps.python.org/pep-0008/#blank-lines)
/// - [Flake 8 rule](https://www.flake8rules.com/rules/E303.html)
/// - [Typing Style Guide](https://typing.readthedocs.io/en/latest/source/stubs.html#blank-lines)
#[violation]
pub struct TooManyBlankLines {
actual_blank_lines: u32,
Expand Down Expand Up @@ -246,9 +261,14 @@ impl AlwaysFixableViolation for BlankLineAfterDecorator {
/// user = User()
/// ```
///
/// ## Typing stub files (`.pyi`)
/// The typing style guide recommends to not use blank lines between statements except to group
/// them. That's why this rule is not enabled in typing stub files.
///
/// ## References
/// - [PEP 8](https://peps.python.org/pep-0008/#blank-lines)
/// - [Flake 8 rule](https://www.flake8rules.com/rules/E305.html)
/// - [Typing Style Guide](https://typing.readthedocs.io/en/latest/source/stubs.html#blank-lines)
#[violation]
pub struct BlankLinesAfterFunctionOrClass {
actual_blank_lines: u32,
Expand Down Expand Up @@ -295,9 +315,14 @@ impl AlwaysFixableViolation for BlankLinesAfterFunctionOrClass {
/// pass
/// ```
///
/// ## Typing stub files (`.pyi`)
/// The typing style guide recommends to not use blank lines between classes and functions except to group
/// them. That's why this rule is not enabled in typing stub files.
///
/// ## References
/// - [PEP 8](https://peps.python.org/pep-0008/#blank-lines)
/// - [Flake 8 rule](https://www.flake8rules.com/rules/E306.html)
/// - [Typing Style Guide](https://typing.readthedocs.io/en/latest/source/stubs.html#blank-lines)
#[violation]
pub struct BlankLinesBeforeNestedDefinition;

Expand Down Expand Up @@ -628,20 +653,23 @@ pub(crate) struct BlankLinesChecker<'a> {
indent_width: IndentWidth,
lines_after_imports: isize,
lines_between_types: usize,
source_type: PySourceType,
}

impl<'a> BlankLinesChecker<'a> {
pub(crate) fn new(
locator: &'a Locator<'a>,
stylist: &'a Stylist<'a>,
settings: &crate::settings::LinterSettings,
source_type: PySourceType,
) -> BlankLinesChecker<'a> {
BlankLinesChecker {
stylist,
locator,
indent_width: settings.tab_size,
lines_after_imports: settings.isort.lines_after_imports,
lines_between_types: settings.isort.lines_between_types,
source_type,
}
}

Expand Down Expand Up @@ -739,6 +767,8 @@ impl<'a> BlankLinesChecker<'a> {
&& !matches!(state.follows, Follows::Docstring | Follows::Decorator)
// Do not trigger when the def follows an if/while/etc...
&& prev_indent_length.is_some_and(|prev_indent_length| prev_indent_length >= line.indent_length)
// Blank lines in stub files are only used for grouping. Don't enforce blank lines.
&& !self.source_type.is_stub()
{
// E301
let mut diagnostic = Diagnostic::new(BlankLineBetweenMethods, line.first_token_range);
Expand All @@ -750,20 +780,31 @@ impl<'a> BlankLinesChecker<'a> {
diagnostics.push(diagnostic);
}

// Blank lines in stub files are used to group definitions. Don't enforce blank lines.
let max_lines_level = if self.source_type.is_stub() {
1
} else {
if line.indent_length == 0 {
BLANK_LINES_TOP_LEVEL
} else {
BLANK_LINES_NESTED_LEVEL
}
};

let expected_blank_lines_before_definition = if line.indent_length == 0 {
// Mimic the isort rules for the number of blank lines before classes and functions
if state.follows.is_any_import() {
// Fallback to the default if the value is too large for an u32 or if it is negative.
// A negative value means that isort should determine the blank lines automatically.
// `isort` defaults to 2 if before a class or function definition and 1 otherwise.
// Defaulting to 2 here is correct because the variable is only used when testing the
// `isort` defaults to 2 if before a class or function definition (except in stubs where it is one) and 1 otherwise.
// Defaulting to 2 (or 1 in stubs) here is correct because the variable is only used when testing the
// blank lines before a class or function definition.
u32::try_from(self.lines_after_imports).unwrap_or(BLANK_LINES_TOP_LEVEL)
u32::try_from(self.lines_after_imports).unwrap_or(max_lines_level)
} else {
BLANK_LINES_TOP_LEVEL
max_lines_level
}
} else {
BLANK_LINES_NESTED_LEVEL
max_lines_level
};

if line.preceding_blank_lines < expected_blank_lines_before_definition
Expand All @@ -775,6 +816,8 @@ impl<'a> BlankLinesChecker<'a> {
&& line.indent_length == 0
// Only apply to functions or classes.
&& line.kind.is_class_function_or_decorator()
// Blank lines in stub files are used to group definitions. Don't enforce blank lines.
&& !self.source_type.is_stub()
{
// E302
let mut diagnostic = Diagnostic::new(
Expand Down Expand Up @@ -804,12 +847,6 @@ impl<'a> BlankLinesChecker<'a> {
diagnostics.push(diagnostic);
}

let max_lines_level = if line.indent_length == 0 {
BLANK_LINES_TOP_LEVEL
} else {
BLANK_LINES_NESTED_LEVEL
};

// If between `import` and `from .. import ..` or the other way round,
// allow up to `lines_between_types` newlines for isort compatibility.
// We let `isort` remove extra blank lines when the imports belong
Expand Down Expand Up @@ -893,6 +930,8 @@ impl<'a> BlankLinesChecker<'a> {
&& line.indent_length == 0
&& !line.is_comment_only
&& !line.kind.is_class_function_or_decorator()
// Blank lines in stub files are used for grouping, don't enforce blank lines.
&& !self.source_type.is_stub()
{
// E305
let mut diagnostic = Diagnostic::new(
Expand Down Expand Up @@ -933,6 +972,8 @@ impl<'a> BlankLinesChecker<'a> {
&& prev_indent_length.is_some_and(|prev_indent_length| prev_indent_length >= line.indent_length)
// Allow groups of one-liners.
&& !(matches!(state.follows, Follows::Def) && line.last_token != TokenKind::Colon)
// Blank lines in stub files are only used for grouping. Don't enforce blank lines.
&& !self.source_type.is_stub()
{
// E306
let mut diagnostic =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---

Loading

0 comments on commit af6ea2f

Please sign in to comment.