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] Do not trigger E3 rules on defs following a function/method with a dummy body #10704

Merged
merged 8 commits into from
Apr 15, 2024
33 changes: 33 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pycodestyle/E30.py
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,39 @@ def test():
# end


# no error
class Foo:
"""Demo."""

@overload
def bar(self, x: int) -> int: ...
@overload
def bar(self, x: str) -> str: ...
def bar(self, x: int | str) -> int | str:
return x
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
# end


# no error
@overload
def foo(x: int) -> int: ...
@overload
def foo(x: str) -> str: ...
def foo(x: int | str) -> int | str:
if not isinstance(x, (int, str)):
raise TypeError
return x
# end


# no error
def foo(self, x: int) -> int: ...
def bar(self, x: str) -> str: ...
def baz(self, x: int | str) -> int | str:
return x
# end


# E301
class Class(object):

Expand Down
24 changes: 21 additions & 3 deletions crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -635,11 +635,19 @@ enum Follows {
Other,
Decorator,
Def,
/// A function whose body is a dummy (...), if the ellipsis is on the same line as the def.
DummyDef,
Import,
FromImport,
Docstring,
}

impl Follows {
const fn is_any_def(self) -> bool {
matches!(self, Follows::Def | Follows::DummyDef)
}
}

impl Follows {
const fn is_any_import(self) -> bool {
matches!(self, Follows::Import | Follows::FromImport)
Expand Down Expand Up @@ -769,6 +777,10 @@ impl<'a> BlankLinesChecker<'a> {
}
}

if logical_line.last_token == TokenKind::Ellipsis {
state.follows = Follows::DummyDef;
}
hoel-bagard marked this conversation as resolved.
Show resolved Hide resolved

if logical_line.is_docstring {
state.follows = Follows::Docstring;
}
Expand Down Expand Up @@ -801,7 +813,9 @@ impl<'a> BlankLinesChecker<'a> {
// Only applies to methods.
&& matches!(line.kind, LogicalLineKind::Function | LogicalLineKind::Decorator)
// Allow groups of one-liners.
&& !(matches!(state.follows, Follows::Def) && !matches!(line.last_token, TokenKind::Colon))
&& !(state.follows.is_any_def() && line.last_token != TokenKind::Colon)
// Allow a function/method to follow a function/method with a dummy body.
&& !matches!(state.follows, Follows::DummyDef)
&& matches!(state.class_status, Status::Inside(_))
// The class/parent method's docstring can directly precede the def.
// Allow following a decorator (if there is an error it will be triggered on the first decorator).
Expand Down Expand Up @@ -852,7 +866,9 @@ impl<'a> BlankLinesChecker<'a> {
// Allow following a decorator (if there is an error it will be triggered on the first decorator).
&& !matches!(state.follows, Follows::Decorator)
// Allow groups of one-liners.
&& !(matches!(state.follows, Follows::Def) && !matches!(line.last_token, TokenKind::Colon))
&& !(state.follows.is_any_def() && line.last_token != TokenKind::Colon)
// Allow a function/method to follow a function/method with a dummy body.
&& !matches!(state.follows, Follows::DummyDef)
// Only trigger on non-indented classes and functions (for example functions within an if are ignored)
&& line.indent_length == 0
// Only apply to functions or classes.
Expand Down Expand Up @@ -1018,7 +1034,9 @@ impl<'a> BlankLinesChecker<'a> {
// Do not trigger when the def/class follows an "indenting token" (if/while/etc...).
&& 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)
&& !(state.follows.is_any_def() && line.last_token != TokenKind::Colon)
// Allow a function/method to follow a function/method with a dummy body.
&& !matches!(state.follows, Follows::DummyDef)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It might be worth extracting this check into a small helper function to avoid repeating it three times (and it gives as the opportunity to give it a meaningful name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 6751ef1.

// Blank lines in stub files are only used for grouping. Don't enforce blank lines.
&& !self.source_type.is_stub()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,83 +1,81 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---
E30.py:453:5: E301 [*] Expected 1 blank line, found 0
E30.py:486:5: E301 [*] Expected 1 blank line, found 0
|
451 | def func1():
452 | pass
453 | def func2():
484 | def func1():
485 | pass
486 | def func2():
| ^^^ E301
454 | pass
455 | # end
487 | pass
488 | # end
|
= help: Add missing blank line

ℹ Safe fix
450 450 |
451 451 | def func1():
452 452 | pass
453 |+
453 454 | def func2():
454 455 | pass
455 456 | # end
483 483 |
484 484 | def func1():
485 485 | pass
486 |+
486 487 | def func2():
487 488 | pass
488 489 | # end

E30.py:464:5: E301 [*] Expected 1 blank line, found 0
E30.py:497:5: E301 [*] Expected 1 blank line, found 0
|
462 | pass
463 | # comment
464 | def fn2():
495 | pass
496 | # comment
497 | def fn2():
| ^^^ E301
465 | pass
466 | # end
498 | pass
499 | # end
|
= help: Add missing blank line

ℹ Safe fix
460 460 |
461 461 | def fn1():
462 462 | pass
463 |+
463 464 | # comment
464 465 | def fn2():
465 466 | pass
493 493 |
494 494 | def fn1():
495 495 | pass
496 |+
496 497 | # comment
497 498 | def fn2():
498 499 | pass

E30.py:474:5: E301 [*] Expected 1 blank line, found 0
E30.py:507:5: E301 [*] Expected 1 blank line, found 0
|
473 | columns = []
474 | @classmethod
506 | columns = []
507 | @classmethod
| ^ E301
475 | def cls_method(cls) -> None:
476 | pass
508 | def cls_method(cls) -> None:
509 | pass
|
= help: Add missing blank line

ℹ Safe fix
471 471 | """Class for minimal repo."""
472 472 |
473 473 | columns = []
474 |+
474 475 | @classmethod
475 476 | def cls_method(cls) -> None:
476 477 | pass
504 504 | """Class for minimal repo."""
505 505 |
506 506 | columns = []
507 |+
507 508 | @classmethod
508 509 | def cls_method(cls) -> None:
509 510 | pass

E30.py:486:5: E301 [*] Expected 1 blank line, found 0
E30.py:519:5: E301 [*] Expected 1 blank line, found 0
|
484 | def method(cls) -> None:
485 | pass
486 | @classmethod
517 | def method(cls) -> None:
518 | pass
519 | @classmethod
| ^ E301
487 | def cls_method(cls) -> None:
488 | pass
520 | def cls_method(cls) -> None:
521 | pass
|
= help: Add missing blank line

ℹ Safe fix
483 483 |
484 484 | def method(cls) -> None:
485 485 | pass
486 |+
486 487 | @classmethod
487 488 | def cls_method(cls) -> None:
488 489 | pass


516 516 |
517 517 | def method(cls) -> None:
518 518 | pass
519 |+
519 520 | @classmethod
520 521 | def cls_method(cls) -> None:
521 522 | pass