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

Harmonize E301 check with format in --preview mode? #10211

Closed
jab opened this issue Mar 3, 2024 · 9 comments · Fixed by #10704
Closed

Harmonize E301 check with format in --preview mode? #10211

jab opened this issue Mar 3, 2024 · 9 comments · Fixed by #10704
Labels
bug Something isn't working incompatibility Incompatibility between different Ruff tools or rules (formatter, isort, linter) preview Related to preview mode features

Comments

@jab
Copy link
Contributor

jab commented Mar 3, 2024

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
@MichaReiser MichaReiser added bug Something isn't working preview Related to preview mode features labels Mar 4, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Mar 4, 2024

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

@hoel-bagard
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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

@jab
Copy link
Contributor Author

jab commented Mar 4, 2024

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
Copy link
Contributor Author

jab commented Mar 4, 2024

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

@MichaReiser
Copy link
Member

MichaReiser commented Mar 4, 2024

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?

@zanieb zanieb added the incompatibility Incompatibility between different Ruff tools or rules (formatter, isort, linter) label Mar 11, 2024
@randolf-scholz
Copy link

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.

@dhruvmanila
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working incompatibility Incompatibility between different Ruff tools or rules (formatter, isort, linter) preview Related to preview mode features
Projects
None yet
6 participants