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

Enforce PEP 698 (override decorator) #131

Merged
merged 1 commit into from
Apr 29, 2024
Merged

Conversation

ItsDrike
Copy link
Member

@ItsDrike ItsDrike commented Jun 15, 2023

PEP 698 has introduced a new typing.override decorator, and suggested that type-checkers add an option to enable strictly enforcing the usage of this decorator for any method override in all classes.

This decorator does 3 things for us:

  1. It makes it very easy to immediately see which methods in a class are overriding some inherited method, and which methods are new, introduced by the current class.
  2. Consider a scenario where we have a class Foo, that defines some method my_method, and a class Bar, which inherits from Foo, and overrides this method with some other custom implementation. What if the parent Foo class suddenly renamed the my_method method to my_cool_method. Once this happened, the Bar class would no longer be overriding the my_method, but rather defining it as a completely new method, and inheriting the renamed my_cool_method from Foo. What typing.override does is ensure that methods marked with it are in fact overrides, so in this scenario, if Bar.my_method was marked as override, there would be a typing error, informing you that my_method is not actually inherited from anywhere, and so the override is invalid.
  3. The methods decorated with typing.override will inherit the original docstring, unless another one is explicitly set. This allows us to reduce the repetition of docstrings fairly significantly. This is even recognized by ruff's flake8-docstrings, so no ugly noqas everywhere.

While the 1st point may be nice, it's not hugely important, however the 2nd point this feature brings could be pretty nice to have enforced, and the 3rd point is also quite neat.


This PR was originally only made to see how enforcing this rule would look in the code-base, and I wasn't really sure whether or not to actually enable this enforcement. However, right now, I feel pretty strongly that enabling this is a good idea, and so, the do-not-merge tag from this PR was removed.

@ItsDrike ItsDrike added p: 3 - low This can wait do-not-merge This PR can be reviewed, but cannot be merged now a: internal Related to internal API of the project t: optimization Optimizations to the code (performance improvements, code cleanup, or other general optimizations) labels Jun 15, 2023
@ItsDrike ItsDrike force-pushed the enforce-typing-override branch 2 times, most recently from 9b206ff to 82ba3c6 Compare June 21, 2023 09:17
@ItsDrike ItsDrike force-pushed the enforce-typing-override branch from 82ba3c6 to c4b893d Compare April 29, 2024 20:37
@ItsDrike ItsDrike removed the do-not-merge This PR can be reviewed, but cannot be merged now label Apr 29, 2024
@ItsDrike ItsDrike force-pushed the enforce-typing-override branch from c4b893d to a796adf Compare April 29, 2024 20:51
This enables the `reportImplicitOverride` pyright toggle, enabling the
enforcement that any overridden methods in classes be marked with the
`typing.override` (or pre 3.12, with `typing_extensions.override`)
@ItsDrike ItsDrike force-pushed the enforce-typing-override branch from a796adf to 2532d35 Compare April 29, 2024 20:55
@ItsDrike ItsDrike merged commit adb5f8c into main Apr 29, 2024
13 checks passed
@ItsDrike ItsDrike deleted the enforce-typing-override branch April 29, 2024 20:58
@ItsDrike ItsDrike mentioned this pull request Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: internal Related to internal API of the project p: 3 - low This can wait t: optimization Optimizations to the code (performance improvements, code cleanup, or other general optimizations)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant