python-makefun icon indicating copy to clipboard operation
python-makefun copied to clipboard

Use `__signature__` and set `__wrapped__` attribute even on signature-changing decorators.

Open lucaswiman opened this issue 3 years ago • 7 comments
trafficstars

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.

lucaswiman avatar Jun 25 '22 07:06 lucaswiman

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 _test_py37. Otherwise if you can get rid of dataclasses and attribute-annotations (and replace the test class with a standard class with annotations in the init method), then you can move it to _test_py35.py. Once you've done this, please add an appropriate pytest skip mark on your test, and import the module inside the test - similar to https://github.com/smarie/python-makefun/blob/de9caee62cdd2835185ae162173e43da7438b956/tests/test_advanced.py#L222-L233

This should make the build pass.

smarie avatar Jun 25 '22 12:06 smarie

This should be ready for another test suite run and final review. Thanks for the very speedy review!

lucaswiman avatar Jun 25 '22 18:06 lucaswiman

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.

lucaswiman avatar Jun 25 '22 18:06 lucaswiman

Note: the test doesn't work on python <3.7.6, because the __wrapped__ dereferencing in get_type_hints was not added until then. See https://github.com/python/cpython/blob/v3.7.6/Lib/typing.py#L986-L987.

lucaswiman avatar Jun 25 '22 19:06 lucaswiman

OK, should be passing on all supported versions of python. Thank you for your patience.

nox > Ran multiple sessions:
nox > * tests-3.9: success
nox > * tests-3.8: success
nox > * tests-2.7: success
nox > * tests-3.5: success
nox > * tests-3.6: success
nox > * tests-3.7: success
nox > * flake8: success

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.

lucaswiman avatar Jun 25 '22 19:06 lucaswiman

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 doc/api_reference.md with the same docstring mod than the ones you did in the code ? Indeed this is not an automated operation at the moment. If I'm not mistaken you just modified the docstring from wraps, but you should probably also modify the docstring from create_function.

Thanks again, almost there :)

smarie avatar Jun 29 '22 14:06 smarie

@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!

lucaswiman avatar Jul 05 '22 18:07 lucaswiman

No worries. Thanks very much!

lucaswiman avatar Sep 07 '22 19:09 lucaswiman