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

Pass documentation around. #3656

Closed
wants to merge 1 commit into from
Closed

Conversation

Natim
Copy link

@Natim Natim commented Feb 17, 2021

Fixes #3655

Use wraps which also link the documentation.

src/pyramid/decorator.py Outdated Show resolved Hide resolved
@luhn
Copy link
Contributor

luhn commented Feb 17, 2021

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
Copy link
Author

Natim commented Feb 17, 2021

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

@@ -36,6 +36,7 @@ class reify:
def __init__(self, wrapped):
self.wrapped = wrapped
update_wrapper(self, wrapped)
self.__doc__ = wrapped.__doc__
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update_wrapper is supposed to do it 😕

Copy link
Member

@mmerickel mmerickel Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty clear, as you said, that update_wrapper is supposed to do it and is also more defensive about doing it than this code (i.e. if wrapped doesn't have a doc attr for some insane reason). I'd want to know why the expected version isn't working first if you could step through update_wrapper and determine why it's not setting this itself.

Copy link
Author

@Natim Natim Feb 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that's why, this would set it to None if not defined which would help Sphinx to find it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this there's no tests for None, it's just pulling the value and setting it.

https://github.com/python/cpython/blob/3.9/Lib/functools.py#L50-L56

@luhn
Copy link
Contributor

luhn commented Feb 17, 2021

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
Copy link
Contributor

luhn commented Feb 17, 2021

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
Copy link
Author

Natim commented Feb 17, 2021

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
Copy link
Contributor

luhn commented Feb 18, 2021

Made the change in #3657

@digitalresistor
Copy link
Member

Closing this in favor of #3657. Thanks @Natim for the initial report!

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
4 participants