-
-
Notifications
You must be signed in to change notification settings - Fork 33.3k
Fixed #36712 -- Evaluated type annotations lazily in template tag registration. #20340
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
b057c2c to
0e1c427
Compare
…istration. Ideally, this will be reverted when an upstream solution is available for python/cpython#141560. Thanks Patrick Rauscher for the report and Augusto Pontes for the first iteration and test.
0e1c427 to
e79c98f
Compare
nessita
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.
Looks good, added a few naming suggestions. Thank you! 🌟
| annotation_format=annotationlib.Format.FORWARDREF, | ||
| ) | ||
|
|
||
| lock = threading.Lock() |
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.
Should this lock also be inside the if PY314: guard?
| yield | ||
| return | ||
| with lock: | ||
| helper_was_already_replaced = False |
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 would suggest using the slightly shorter:
| helper_was_already_replaced = False | |
| helper_was_replaced = False |
|
|
||
|
|
||
| @contextmanager | ||
| def leave_deferred_annotations_unevaluated(): |
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.
How about:
| def leave_deferred_annotations_unevaluated(): | |
| def lazy_deferred_annotations(): |
| * Fixed a bug where template tag functions relying on the Python 3.14 behavior | ||
| of deferred evaluation of type annotations could not be registered | ||
| (:ticket:`36712`). |
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 the bug in 5.2? or in 5.2.8? ideally we should be explicit about that. Also how about:
| * Fixed a bug where template tag functions relying on the Python 3.14 behavior | |
| of deferred evaluation of type annotations could not be registered | |
| (:ticket:`36712`). | |
| * Fixed a crashed in Django 5.2 and Python 3.14+ that prevented template tag | |
| functions from being registered when their type annotations required deferred | |
| evaluation (:ticket:`36712`). |
Trac ticket number
ticket-36712
Branch description
Python 3.14 introduces deferred evaluation of type annotations, which can make headaches around circular definitions go away.
inspect.getfullargspecwasn't updated to take the newannotation_formatarg that was provided to let inspecting libraries (us) avoid eagerly evaluating user annotations, see python/cpython#141560.One solution would have been to copy and paste the entire function--along with importing the seven or eight other symbols it needed--and do the tweak, long-term forking the implementation.
Instead, patching the module helper anticipating the change we want Python to make felt more future-proof and sufficiently self-documenting. However, I'm happy to swap this out for a copy-and-paste approach if that's preferable, but we'd want to redistribute the python license in that case, I suppose?
Ideally, this will be reverted when an upstream solution is available for python/cpython#141560.
Thanks Patrick Rauscher for the report and Augusto Pontes for the first iteration and test.
Checklist
mainbranch.