bpo-37838: get_type_hints for wrapped functions with forward reference by benedwards14 · Pull Request #17126 · python/cpython
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good change to me! Just a few comments below to fix CI and clean things up.
| def dec(func): | ||
| def wrapper(*args, **kwargs): | ||
| return func(*args, **kwargs) | ||
| return wraps(func)(wrapper) No newline at end of file |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the tests are failing because of this missing newline:
| return wraps(func)(wrapper) | |
| return wraps(func)(wrapper) | |
|
|
||
| class ForRefExample(): | ||
| @ann_module.dec | ||
| def func(a: 'ForRefExample'): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but maybe just name this argument self for cleanliness?
| def func(a: 'ForRefExample'): | |
| def func(self: 'ForRefExample'): |
|
|
||
| @ann_module.dec | ||
| @ann_module.dec | ||
| def nested(a: 'ForRefExample'): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above:
| def nested(a: 'ForRefExample'): | |
| def nested(self: 'ForRefExample'): |
| self.assertEqual(gth(G), {'lst': ClassVar[List[T]]}) | ||
|
|
||
| def test_get_type_hints_wrapped_decoratored_func(self): | ||
| expects = {'a': ForRefExample} |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above:
| expects = {'a': ForRefExample} | |
| expects = {'self': ForRefExample} |
Thanks for the PR @benedwards14!
This one should also have a NEWS entry. Just something simple, like:
:meth:`typing.get_type_hints` properly handles functions decorated with :meth:`functors.wraps`.
Benjamin Edwards and others added 2 commits
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a couple of remaining style nitpicks:
|
|
||
| gth = get_type_hints | ||
|
|
||
| class ForRefExample(): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No parentheses here, and 2 blank lines between top-level definitions:
| class ForRefExample(): | |
| class ForRefExample: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, one last typo I spotted. Then good!
Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
(Added backport labels since this is a bugfix.)
Thanks @benedwards14 for the PR, and @ilevkivskyi for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request
