Skip to content

Comments

Add strict_override_decorator option (PEP 698)#15512

Merged
JukkaL merged 7 commits intopython:masterfrom
cdce8p:override-strict-mode
Jul 13, 2023
Merged

Add strict_override_decorator option (PEP 698)#15512
JukkaL merged 7 commits intopython:masterfrom
cdce8p:override-strict-mode

Conversation

@cdce8p
Copy link
Collaborator

@cdce8p cdce8p commented Jun 24, 2023

Add the strict mode for PEP 698.

Closes: #14072

@cdce8p cdce8p force-pushed the override-strict-mode branch from 8ec9cb4 to 0827dcd Compare June 24, 2023 21:17
@cdce8p cdce8p mentioned this pull request Jun 24, 2023
Closed
2 tasks
@github-actions

This comment has been minimized.

Sign in to view
emmatyping
emmatyping reviewed Jun 26, 2023
View reviewed changes
emmatyping
emmatyping approved these changes Jun 26, 2023
View reviewed changes
Copy link
Member

@emmatyping emmatyping 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!

tmke8
tmke8 reviewed Jun 27, 2023
View reviewed changes
@cdce8p cdce8p force-pushed the override-strict-mode branch from 0827dcd to de4590e Compare June 27, 2023 14:19
@github-actions

This comment has been minimized.

Sign in to view
@cdce8p cdce8p force-pushed the override-strict-mode branch from de4590e to 6e40627 Compare July 5, 2023 08:49
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@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 emmatyping July 5, 2023 10:40
@cdce8p cdce8p mentioned this pull request Jul 5, 2023
Closed
tmke8
tmke8 reviewed Jul 5, 2023
View reviewed changes
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
JukkaL
JukkaL reviewed Jul 11, 2023
View reviewed changes
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.

test-data/unit/check-functions.test

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

@JukkaL JukkaL Jul 11, 2023

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

@cdce8p cdce8p Jul 11, 2023

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.

mypy/test-data/unit/check-functions.test

Lines 2937 to 2947 in 67cc059

[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]

@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

github-actions bot commented Jul 11, 2023

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
JukkaL
JukkaL approved these changes Jul 13, 2023
View reviewed changes
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
@cdce8p cdce8p mentioned this pull request Jul 13, 2023
Merged
20 tasks
@cdce8p cdce8p deleted the override-strict-mode branch July 13, 2023 16:16
@tmke8 tmke8 mentioned this pull request Dec 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JukkaL JukkaL JukkaL approved these changes

@emmatyping emmatyping Awaiting requested review from emmatyping

+1 more reviewer

@tmke8 tmke8 tmke8 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support PEP 698 – Override Decorator

4 participants

@cdce8p @JukkaL @tmke8 @emmatyping