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

Add strict_override_decorator option (PEP 698) #15512

Merged
merged 7 commits into from
Jul 13, 2023

Conversation

cdce8p
Copy link
Collaborator

@cdce8p cdce8p commented Jun 24, 2023

Add the strict mode for PEP 698.

Closes: #14072

@github-actions

This comment has been minimized.

mypy/messages.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ethanhs ethanhs left a comment

Choose a reason for hiding this comment

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

One minor wording suggestion but I think this looks good otherwise!

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@cdce8p
Copy link
Collaborator Author

cdce8p commented Jul 5, 2023

Pushed a new update. The error message now includes the base class name. Similar to pyright.
The PR would be ready now.

@cdce8p cdce8p requested a review from ethanhs July 5, 2023 10:40
mypy/checker.py Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good overall, some suggestions below. This is an important feature to have in mypy.

docs/source/class_basics.rst Outdated Show resolved Hide resolved
docs/source/config_file.rst Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
mypy/main.py Outdated Show resolved Hide resolved
mypy/messages.py Outdated Show resolved Hide resolved
test-data/unit/check-functions.test Show resolved Hide resolved

[case requireExplicitOverrideMultipleInheritance]
# flags: --strict-override-decorator --python-version 3.12
from typing import override
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test using override imported from typing_extensions on an older Python version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rather not do that. Since none of our test fixtures use @override, we inadvertently get errors for those as well. That's also why I added a separate fixture for these test cases with just enough definitions and no accidental overrides. I would have to do the same just for the typing_extensions fixture.

There is also a test case for an unused @override already. As this PR doesn't change the decorator parsing, it probably fine to skip it.

[case explicitOverrideFromExtensions]
from typing_extensions import override
class A:
def f(self, x: int) -> str: pass
class B(A):
@override
def f2(self, x: int) -> str: pass # E: Method "f2" is marked as an override, but no base method was found with this name
[typing fixtures/typing-full.pyi]
[builtins fixtures/tuple.pyi]

docs/source/config_file.rst Outdated Show resolved Hide resolved
@cdce8p
Copy link
Collaborator Author

cdce8p commented Jul 11, 2023

Thanks for the review @JukkaL! Using a new error code instead of a cmd line option does make sense. The change was fairly strait forward. With that I also updated the documentation and added a whole new section in error_code_list2.rst.

For now I used explicit-override as error code. Happy to change that if we can come up with something better though.

@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@cdce8p cdce8p requested a review from JukkaL July 11, 2023 20:57
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, looks good!

@JukkaL JukkaL merged commit dfea43f into python:master Jul 13, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support PEP 698 – Override Decorator
4 participants