Skip to content

Conformance tests: classes_override.py should allow highlighting the decorator #1955

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

Closed
alvov26 opened this issue Mar 31, 2025 · 1 comment · Fixed by #1958
Closed

Conformance tests: classes_override.py should allow highlighting the decorator #1955

alvov26 opened this issue Mar 31, 2025 · 1 comment · Fixed by #1958
Labels
topic: other Other topics not covered

Comments

@alvov26
Copy link
Contributor

alvov26 commented Mar 31, 2025

While working on support for this test in PyCharm, I noticed that it requires highlighting the function signature instead of the decorator (which is how PyCharm currently handles it).
For example:

    @staticmethod
    @override
    def static_method1() -> int:  # E: no matching signature in ancestor
        return 1

    @classmethod
    @override
    def class_method1(cls) -> int:  # E: no matching signature in ancestor
        return 1

This would be easy to change on our end, but I believe allowing the option to highlight the decorator would be preferable for several reasons:

  1. Highlighting the function signature may overlap with other checks, such as PyCharm inspections or other linters.
  2. If the decorator is removed, the error no longer applies, so highlighting the decorator itself makes sense.
  3. Other conformance tests already allow highlighting decorators. For example:
  • qualifiers_final_decorator.py
@final  # E[func]: not allowed on non-method function.
def func1() -> int:  # E[func]
    return 0
  • overloads_definitions.py
@overload  # E[func2]
def func2(x: int) -> int:  # E[func2]: no implementation
    ...

If this change makes sense, I’d be happy to submit a PR to update the test.

@alvov26 alvov26 added the topic: other Other topics not covered label Mar 31, 2025
@JelleZijlstra
Copy link
Member

Makes sense to me, feel free to send a PR.

The exact placement of errors isn't a strict requirement, and we've mostly done the placement on the basis of the behavior of type checkers we're already testing. If it makes sense to use a different line to show the error I'm happy to allow it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: other Other topics not covered
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants