Skip to content
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

Merged
merged 4 commits into from Feb 20, 2021
Merged

Remove update_wrapper from reify #3657

merged 4 commits into from Feb 20, 2021

Conversation

luhn
Copy link
Contributor

@luhn luhn commented Feb 17, 2021

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:

import inspect
from pyramid.decorator import reify
from functools import cached_property
from sphinx.util.inspect import isattributedescriptor


class Widget:
    @property
    def prop(self):
        return 'Foo'

    @cached_property
    def cached(self):
        return 'Foo'

    @reify
    def reify(self):
        return 'Foo'


print(isattributedescriptor(Widget.prop))
print(isattributedescriptor(Widget.cached))
print(isattributedescriptor(Widget.reify))

Output is:

True
True
False

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.

@luhn
Copy link
Contributor Author

luhn commented Feb 17, 2021

So this causes this test to fail.

I'm confused what the test is doing. Why are we reify'ing the viewdefaults wrapper? That seems like a mistake.

@mmerickel
Copy link
Member

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.

@luhn
Copy link
Contributor Author

luhn commented Feb 17, 2021

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?

@luhn
Copy link
Contributor Author

luhn commented Feb 17, 2021

Should this be backported? Technically this isn't a bw compat change, but the differences are subtle.

@mmerickel
Copy link
Member

Not sure why they aren't showing up nicely in github search but they are here:

class Test_view_defaults(unittest.TestCase):

@mmerickel
Copy link
Member

I think it's ok to backport if you want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sphinx doesn't like the reify decorator
2 participants