ruff icon indicating copy to clipboard operation
ruff copied to clipboard

Harmonize E301 check with format in --preview mode?

Open jab opened this issue 1 year ago • 9 comments

ruff format allows overloaded method stubs to occur before the runtime definition with no blank line in between, which is good. But this style fails the E301 check (in preview mode only). Should these be harmonized?

minimal code snippet that reproduces the bug

import typing as t


class Foo:
    """Demo."""

    @t.overload
    def bar(self, x: int) -> int: ...
    @t.overload
    def bar(self, x: str) -> str: ...
    def bar(self, x: int | str) -> int | str:
        return x

commands invoked

❯ ruff format --preview foo.py  # (this example works the same without --preview here too)
1 file left unchanged

❯ ruff check --preview --extend-select E foo.py  # (--preview required here to reproduce)
foo.py:11:5: E301 [*] Expected 1 blank line, found 0
   |
 9 |     @t.overload
10 |     def bar(self, x: str) -> str: ...
11 |     def bar(self, x: int | str) -> int | str:
   |     ^^^ E301
12 |         return x
   |
   = help: Add missing blank line

Found 1 error.
[*] 1 fixable with the `--fix` option.

ruff version

❯ ruff --version
ruff 0.3.0

jab avatar Mar 03 '24 17:03 jab

I don't know how complicated it is to support this, but I think changing E301 to accept this syntax would be good regardless. CC: @hoel-bagard

MichaReiser avatar Mar 04 '24 13:03 MichaReiser

I don't think that would be an easy change, but I'll have a look. One thing to note is that it would deviate from pycodestyle.

❯ pycodestyle foo.py
foo.py:11:5: E301 expected 1 blank line, found 0

hoel-bagard avatar Mar 04 '24 14:03 hoel-bagard

@MichaReiser One way the to accept this syntax would be to keep track of the function name (since we would only allow the syntax if the name of the function is the same as the previous one) by modifying the enums as shown below. Then it should only be a matter of modifying the E301's if condition to not trigger if we have two functions with the same name following each other (here I'm assuming that this would only happen if the first function is decorated with an @overload, if we don't make that assumption then we would need to additionally keep track of the previous two lines).

enum LogicalLineKind {
    ...
    Function,
    ...
}

enum Follows {
    ...
    Def,
    ...
}

to

enum LogicalLineKind {
    ...
    Function(String),
    ...
}

enum Follows {
    ...
    Def(String),
    ...
}

I haven't done it yet, but I can give it a try if you think that's a good idea.

hoel-bagard avatar Mar 04 '24 14:03 hoel-bagard

We could also allow any function to follow a one-liner function. Right now we allow the first of the two snippets below, but not the second one. This is the same behavior as pycodestyle.

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

hoel-bagard avatar Mar 04 '24 14:03 hoel-bagard

Great there is interest in this!

Looks like the same thing applies to E302 with overloaded free functions (not just methods): Screenshot 2024-03-04 at 10 31 28 AM

I'm not very familiar with pycodestyle, but it looks like it's been around since before Python had type hints. Otherwise perhaps it would have accepted (or even required) this style for overloaded functions from the get-go. Given that, maybe there could be a general policy for what to do whenever there is divergence due to type hint related code style that pycodestyle was not designed for (if there isn't one already).

jab avatar Mar 04 '24 15:03 jab

I think ideally the solution here wouldn't be sensitive to how many lines were in the function definition. E.g. This should still be accepted code style:

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

jab avatar Mar 04 '24 15:03 jab

One way the to accept this syntax would be to keep track of the function name (since we would only allow the syntax if the name of the function is the same as the previous one)

I'm sorry. I should have linked to the formatter issue so you don't have to infer the formatter's rules (which can be difficult to debug). The style change is called dummy implementations and it makes the blank lines between two functions optional if the preceding function has a dummy (...) body. It isn't required that the two functions have the same name or that one is marked with @overloaded.

Which is what you're proposing here:

We could also allow any function to follow a one-liner function. Right now we allow the first of the two snippets below, but not the second one. This is the same behavior as pycodestyle.

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

I think it's a good compromise between pycodestyle and formatter compatibility (and it seems a sensible defaults for stubs to me). The lint rule is allowed to be slightly stricter than the formatter (the formatter makes the blank line optional, it doesn't remove it). That means we could enforce that the functions have the same name, but I don't think that's necessary. @jab what's your take on whether the functions should have the same name?

Would we need to make the same change for methods?

MichaReiser avatar Mar 04 '24 17:03 MichaReiser

When formatting with ruff, is there even a point in having these checks active? Maybe rules could be categorized with some sort of tags, so that one can easily disable all formatting-specific rules, or activate specific rule sets like format:pycodestyle, format:ruff, format:black, etc.

randolf-scholz avatar Mar 31 '24 17:03 randolf-scholz

@randolf-scholz Thank you for the suggestion! @AlexWaygood is working towards rule categorization (#1774) where it would make sense to have this kind of group.

dhruvmanila avatar Apr 01 '24 08:04 dhruvmanila