Pylons / pyramid

Pyramid - A Python web framework
https://trypyramid.com/
Other
3.97k stars 887 forks source link

Pass documentation around. #3656

Closed Natim closed 3 years ago

Natim commented 3 years ago

Fixes #3655

Use wraps which also link the documentation.

luhn commented 3 years ago

I double checked the docs and I don't see anything wrong with our usage of update_wrapper. wraps is meant to be a decorator, and since it's not being used as one here I think it's a no-op.

Natim commented 3 years ago

@luhn update_wrapper is meant to be a decorator too: https://github.com/python/cpython/blob/master/Lib/functools.py#L25-L78

The difference between the two, is the list of information that are passed around.

Actually I am wrong about that and you are right.

I don't know why this PR fixes it then :confused:

luhn commented 3 years ago

This __doc__ stuff is a red herring.

The issue is that sphinx uses isattributedescriptor to determine if a descriptor represents an attribute or not.

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

So Sphinx doesn't recognize reify as an attribute. I haven't dug into the implementation to figure out what the qualifications for an attribute are.

luhn commented 3 years ago

Funny, I think the problem is that we're using update_wrapper. It's making the descriptor look like the method it's wrapping, but that's specifically not what we want to do because reify doesn't behave like a method, it behaves like an attribute.

So @Natim, your fix using wraps worked because it was a no-op, and getting rid of update_wrapper fixes the problem.

Natim commented 3 years ago

Thank you for your great analysis. Feel free to update the PR if you want or I will be able to do it in about 10 hours. 🙏

luhn commented 3 years ago

Made the change in #3657

digitalresistor commented 3 years ago

Closing this in favor of https://github.com/Pylons/pyramid/pull/3657. Thanks @Natim for the initial report!