gh-119180: Add `annotationlib` module to support PEP 649 by JelleZijlstra · Pull Request #119891 · python/cpython
This PR is still ready for review. While we will likely make some change related to metaclasses, that won't materially effect the changes here. I'd like to get this PR landed as a foundation to build the rest of the PEP implementation on, so I'd appreciate any reviews.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me!
Comment on lines +108 to +111
| if self.__forward_module__ is not None: | ||
| globals = getattr( | ||
| sys.modules.get(self.__forward_module__, None), "__dict__", globals | ||
| ) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this (if set) will override the above heuristics on lines 97-107 anyway, why not move it above line 97 so that if we get a globals from __forward_module__ we don't need to bother with those other lookups?
| def test_expressions(self): | ||
| def f( | ||
| add: a + b, | ||
| sub: a + b, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| anno, | ||
| { | ||
| "add": "a + b", | ||
| "sub": "a + b", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "sub": "a + b", | |
| "sub": "a - b", |
| self.assertEqual(annotationlib.get_annotations(int), {}) | ||
| self.assertEqual(annotationlib.get_annotations(object), {}) | ||
|
|
||
| def test_custom_metaclass(self): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to add tests here for the pathological cases with metaclass annotations, or make that change in a separate PR?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave metaclasses to a separate PR (#119180).
|
|
||
| def test_custom_object_with_annotations(self): | ||
| class C: | ||
| def __init__(self, x: int = 0, y: str = ""): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the arguments and their annotations actually related to the test? If not, I'd remove them for clarity about what behavior the test is actually specifying.
Thanks @carljm for the review! I pushed fixes for the issues you identified.
nohlson pushed a commit to nohlson/cpython that referenced this pull request
…n#119891) Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
nohlson pushed a commit to nohlson/cpython that referenced this pull request
…n#119891) Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>