-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
gh-141388: Improve support for non-function callables as annotate functions #142327
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
base: main
Are you sure you want to change the base?
Conversation
… __defaults__, and __kwdefaults__
sobolevn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not quite convinced that this should be done. __annotate__ is already complex enough.
In the issue itself there were no real use-cases that can support the idea.
In my opinion, it is fine to have __annotate__ magic function as function only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really prefer this version where we leave most of the work to the user because it's an advanced feature that they need to respect. But I also don't know whether it's worth it. It kinda slows down annotate functions for everyone. Maybe just documenting what needs to be present is sufficient but we maybe just say "Your callable should behave like a function, with the same dunders".
Doc/library/annotationlib.rst
Outdated
| class Annotate: | ||
| called_formats = [] | ||
| def __call__(self, format=None, *, _self=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the signature we want to force?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we're not really forcing it, but you're right that it is a somewhat awkward suggestion. I'll have a think about alternatives.
We could also move the logic in __call__ to a separate method and then do the self and _self handling within __call__ before passing to the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, at this point, I prefer that we force users to use functions and not arbitrary callable. This additional cost doesn't seem right, it complicates the docs, and makes the code more complex as well.
|
Seems like there's some consensus that it's not worth supporting non-functions any more than they currently are. I still think it's worth supporting them (see #141388), but if it's left as a bit of hassle, then that's alright since it's an advanced feature. I'll update the NEWS entry and add the missing attributes to docs soon when I have some time :) |
|
Well, I said that we would break anyone wanting to use a class for encapsulation purposes but now that I see what kind of work and additional docs it entails, I'm actually more worried about the implementation itself. If we were to actually support this kind of work, maybe a PyPI package would be better which would be able to turn an arbitrary callable into a correct |
call_annotate_function()tries to run with fake globals but can't find the annotate function's__code__attribute.__defaults__, that's fine, just assume it'sNone.If required, I can remove some (or all) of these defaults, particularly
__globals__which adds an extra parameter to_build_closure(), or__builtins__which is semantically correct but a little more complicated than the rest (it would be much simpler to just fallback tobuiltins.__dict__).annotationlibdocumentation providing a simple recipe for a 'minimum viable implementation' of a custom callable class as annotate function. Happy to remove if this is unnecessary/too niche - I just thought it was the clearest way of demonstrating how to implement one of these, without just spelling out which dunders they need to implement.__annotate__Functions don't actually need to be functions #141388📚 Documentation preview 📚: https://cpython-previews--142327.org.readthedocs.build/