-
Notifications
You must be signed in to change notification settings - Fork 13
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
Use __signature__
and set __wrapped__
attribute even on signature-changing decorators.
#86
Conversation
I am not sure the fix for smarie#66 was correct given that __wrapped__ is used by the typing module in get_type_hints, and setting __signature__ seems to fix all other tests.
Very nice proposal @lucaswiman ! It seems that your test code is not compliant with python 2, 3.5, and 3.6, mostly because you use dataclasses and type hints in the dataclass. I guess this is optional, since forward refs and type hints exist since 3.5 If you believe that this is not worth trying to pass on python 3.6 or 3.5, please move your test module contents in a new file named python-makefun/tests/test_advanced.py Lines 222 to 233 in de9caee
This should make the build pass. |
Co-authored-by: Sylvain Marié <sylvain.marie@schneider-electric.com>
Co-authored-by: Sylvain Marié <sylvain.marie@schneider-electric.com>
Co-authored-by: Sylvain Marié <sylvain.marie@schneider-electric.com>
1. Always setting __wrapped__ means we needed a slightly different assertion. 2. Support python <3.7 in the test suite.
b71d0fb
to
d9e9e3f
Compare
This should be ready for another test suite run and final review. Thanks for the very speedy review! |
Whoops, I missed the instructions in the README about how to actually run the full test suite. I'm working on fixing the test collection bug in 2.7. |
Note: the test doesn't work on python <3.7.6, because the |
1. Rename issue_85_module so that it is not collected by pytest. 2. Use the signature polyfill rather than inspect.signature.
OK, should be passing on all supported versions of python. Thank you for your patience.
I noticed that 3.10 is now available on conda. The tests pass on 3.10 as well. You may want to add 3.10 to the build. |
Thanks @lucaswiman ! Sorry to be picky, but I just realized one important thing (see comment in code). Also could you please include in the PR an update to the Thanks again, almost there :) |
@smarie I believe I addressed your review comments. Please let me know if those changes were insufficient or if you need any other updates. Thanks very much! |
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.
Thanks @lucaswiman for this nice PR! Sorry for the wait
No worries. Thanks very much! |
Fixes #85 by using the
__signature__
attribute specified by PEP 362 where appropriate, and setting the__wrapped__
attribute in all cases.I think the fix for #66 introduced an issue with using forwardrefs in type annotations. Since
typing
uses__wrapped__
to dereference forward references, it's can lead to inconsistent behavior (https://github.com/python/cpython/blob/605e9c66ad367b54a847f9fc65447a071742f554/Lib/typing.py#L2314-L2315), not setting it seems inappropriate.Always setting
__wrapped__
and setting__signature__
when the signature changes seems to fix all the issues with typing and docs/etc.Testing
I added a failing test for #85, asserting that
typing.get_type_hints
works correctly with signature-changing decorator on a method with a forward reference in its type annotations. It passes after my fix.