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

Fix typos in PEP 698 #2830

Merged
merged 1 commit into from
Oct 11, 2022
Merged

Fix typos in PEP 698 #2830

merged 1 commit into from
Oct 11, 2022

Conversation

mkorpela
Copy link
Contributor

Fix some typos. Would like to give some comments to the PEP as the maintainer of overrides package, but I'll do it on reddit for example.

Fix some typos. Would like to give some comments to the PEP as the maintainer of overrides package, but I'll do it on reddit for example.
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Oct 11, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks! cc @stroxler but I'll merge this once CI is green because the typos are obvious.

@AA-Turner AA-Turner changed the title Update pep-0698.rst Fix typos in PEP 698 Oct 11, 2022
Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

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

Thanks!

@mkorpela
Copy link
Contributor Author

My comments about the PEP https://www.reddit.com/r/Python/comments/xmnv04/comment/irw9prw/?utm_source=share&utm_medium=web2x&context=3
"Thanks for making this PEP 698 about override decorator! I'm the maintainer of overrides package and would like to comment on few aspects here.

The package https://github.com/mkorpela/overrides exists basically because this functionality has been missing from Python and was in many other languages that I had previously used. First version dates back to 2011 and this answer to stackoverflow.com/a/8313042 . Based on usage statistics of the package, I think the functionality is shown to be valuable.

The PEP 698 proposes a decorator with no runtime impact at all ( similar to typing module final pep-0591 ). When final was introduced to typing module, I tried to integrate it to overrides package overrides/issues/49. The problem was that there seems to be no runtime trace of the decorator. This means it can not be used. Calling the decorator inside of my own decorator did not seem to help.

I think when using a decorator as an annotation, there should be a runtime trace of its existence. Leaving no metadata trace, will mean no ability to use it for dynamic reasoning about the code during runtime. I think dynamic reasoning is an important ability in Python. In overrides case dynamic reasoning is used to copy doc strings from the super class method to the method that is overriding it."

@JelleZijlstra JelleZijlstra merged commit 67e29bf into python:main Oct 11, 2022
@stroxler
Copy link
Contributor

My reaction (https://www.reddit.com/r/Python/comments/xmnv04/comment/irwbk7e/?utm_source=share&utm_medium=web2x&context=3)

Our main concern was the order of decorators, which is tricky to get right in all cases.

In particular, to get reliable runtime behavior @override would have to go above all function-based decorators and below all descriptor-based decorators.

That's not something a user can get right unless they know the implementation of every decorator they use on an override method. We felt like that's a pretty high barrier to entry, since there's no signal about whether a decorator is wrapper- or descriptor-based in the syntax.

I guess this brings me to two questions:

  • would you be fine with us allowing out-of-order @override in type checkers, so that it is entirely the user's responsibility to get the ordering right if they want runtime behavior? One solution would be to provide the runtime behavior, and just document in the PEP that (a) decorator ordering is tricky and (b) type checkers are not going to validate the order

  • Would it help if type checkers also support @overrides.overrides for static checks?

@mkorpela
Copy link
Contributor Author

mkorpela commented Oct 11, 2022

would you be fine with us allowing out-of-order @override in type checkers, so that it is entirely the user's responsibility to get the ordering right if they want runtime behavior? One solution would be to provide the runtime behavior, and just document in the PEP that (a) decorator ordering is tricky and (b) type checkers are not going to validate the order

Yes I would be fine. The functionality as a separate package has been around for years.

Would it help if type checkers also support @overrides.overrides for static checks?

Maybe for some transition period 🤔 .
I think after / if this PEP 698 happens, then overrides packages original purpose and lifetime is getting near to an end. Maybe there might be a reason for it to exist because of the doc strings but post 3.12 world eventually I think the package should give users an easy way to migrate to typing.override (assuming PEP goes through).
3.11 and comment here https://www.reddit.com/r/Python/comments/xmnv04/comment/irwow45/?utm_source=share&utm_medium=web2x&context=3 point that "Changed in version 3.11: The decorator will now set the final attribute to True on the decorated object. " which now means that I can change the final decorator https://github.com/mkorpela/overrides/blob/main/overrides/final.py to use directly typing.final with 3.11

@CAM-Gerlach
Copy link
Member

FYI, the PEP discussion thread would likely be a better place to discuss this than a PR on the PEPs repo fixing typos...but I don't see it linked in the PEP (I'm guessing it is maybe on Typing-SIG somewhere)? @JelleZijlstra do you think you could add it to the appropriate headers?

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.

5 participants