astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
31.71k stars 1.07k forks source link

`ANN001` and `ANN201` for override methods? #9352

Open ReubenFrankel opened 9 months ago

ReubenFrankel commented 9 months ago

When overriding a method from a parent class, is it required to define the entire original method signature with its argument and return types? I'm not sure if this is a personal preference thing, but I don't really want to have to redefine or import any non-builtin types if I don't have to (especially for more complex methods) - VS Code IntelliSense seems to pick these up already without any type hints.

Given the following example

example.py

"""Example module."""

from singer_sdk.streams.rest import RESTStream
from typing_extensions import override

class MyRESTStream(RESTStream):
    """My RESTStream implementation."""

    @override
    def prepare_request(self, context, next_page_token):
        return super().prepare_request(context, next_page_token)

where RESTStream.prepare_request is a fully-annotated method...

Expected

No errors

Actual

example.py:11:9: ANN201 Missing return type annotation for public function `prepare_request`
example.py:11:31: ANN001 Missing type annotation for function argument `context`
example.py:11:40: ANN001 Missing type annotation for function argument `next_page_token`
Found 3 errors.

This is the code that fixes the above 3 errors and some others after importing:

example.py

"""Example module."""

from __future__ import annotations

from typing import TYPE_CHECKING, Any

from singer_sdk.streams.rest import RESTStream
from typing_extensions import override

if TYPE_CHECKING:
    from requests.models import PreparedRequest

class MyRESTStream(RESTStream):
    """My RESTStream implementation."""

    @override
    def prepare_request(
        self,
        context: dict | None,
        next_page_token: Any | None,
    ) -> PreparedRequest:
        return super().prepare_request(context, next_page_token)
charliermarsh commented 9 months ago

I'd be interested to get more opinions on this, I can see both sides.

tylerlaprade commented 2 weeks ago

I came searching for this because after adding ANN201, I have to add Response to every single one of my overridden Django ViewSet methods when the type-checker already knows they always have to return a Response.

zanieb commented 2 weeks ago

cc @AlexWaygood / @MichaReiser

Do our plans for red-knot make this clearer? I basically think we should not enforce annotations for overrides with these rules.

MichaReiser commented 2 weeks ago

Do our plans for red-knot make this clearer? I basically think we should not enforce annotations for overrides with these rules.

I don't think the decision here is red knot specific. Red knot will provide better means to reason about overrides but it's a non-goal for the first release to improve any lint rules.

I do agree that requiring type annotations on overrides is overly strict but not requiring them has its own downsides, unless Ruff has other rules that fill the gaps:

I could see us going middle ground here where a method decorated with override should either be fully annotated or not annotated at all.

tylerlaprade commented 2 weeks ago

All three of those bullet points will be covered by the type-checker. I wouldn't expect a linter to have an opinion on them.