-
Notifications
You must be signed in to change notification settings - Fork 887
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
Remove update_wrapper from reify #3657
Conversation
So this causes this test to fail. I'm confused what the test is doing. Why are we reify'ing the |
Huh, yeah so I traced that test back to 52fde9a. It looks to me like I totally screwed up the test case when porting it to the new location in the codebase. |
Since it looks like the original intent was to test reify, I've removed the failing test and added some new tests for reify. However, I don't see any tests dedicated to viewdefaults... Am I overlooking them or do we need to write some? |
Should this be backported? Technically this isn't a bw compat change, but the differences are subtle. |
Not sure why they aren't showing up nicely in github search but they are here: Line 917 in ee7ca28
|
I think it's ok to backport if you want to. |
Reify uses
update_wrapper
, which makes the descriptor look like the wrapped method. However, this isn't the desired behavior, because reify changes the method to act like the property.This causes problems with Sphinx autodoc as reported in #3655, because Sphinx sees the reify'd attribute as a method and throws an error when attempting to get a signature. You can see the problem here:
Output is:
This fix removes
update_wrapper
from reify and just sets__doc__
manually. This is in line with stdlib's implementation of functools.cached_property.Fixes #3655.