-
-
Notifications
You must be signed in to change notification settings - Fork 373
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
slots + cached_property + sphinx #1325
Comments
a Short, Self Contained, Correct, Example would be great; my guess is that we’re wrapping the method and probably don’t copy over the docstring. maybe it’s just a matter of calling which, of course, you’re very much free to try out and report back 😇 |
Hi @hynek, thanks for the quick answer. Seems exactly to be the case! from functools import cached_property
from attrs import define
@define
class A:
@property
def x() -> int:
"""ABC"""
return 0
@cached_property
def y() -> int:
"""DEF"""
return 0
print(A.x.__doc__)
print(A.y.__doc__) The first one correctly yields "ABC", the second is |
Hmm, given how this is implemented (descriptors galore), I'm afraid this is not gonna be easy. I'm open to PRs, tho. Here's a failing test case: def test_slots_cached_property_retains_doc():
"""
Cached property's docstring is retained.
"""
@attr.s(slots=True)
class A:
x = attr.ib()
@functools.cached_property
def f(self):
"""
This is a docstring.
"""
return self.x
assert "This is a docstring." in A.f.__doc__ |
@diabolo-dan: I saw that you added the support in #1200. Do you see any easy solution to the problem? |
Hmm, having had a brief look, I don't think there's a solution compatible with the current approach. The problem is that slots are implemented with a C based descriptor, and that, afaict, you can't set a doc on them. There is an alternative approach which involves wrapping the |
As far as I am concerned, I think we can afford an extra function call if it makes the whole thing a bit rounder. CodSpeed is gonna yell at us, if the penalty is too excessive. |
That would be really great! I think otherwise the behavior (and especially the error message) would be really surprising to users. @diabolo-dan: Is this something you'd be willing to take care of? (Since, honestly, I have zero clue at the moment about how the |
@diabolo-dan: ping 🙃 Let me know if you are too busy, I hope we can then find another way |
@diabolo-dan: one more attempt here 🦤 I think otherwise there is not much progress to be expected on this topic ... |
Hey, sorry for the delay, I started taking a look at this, and you can see a rough approach here: One issue is that it needs to check all (attrs) super-classes for cached properties (even if it has none itself, in case one is overridden, e.g. with a normal slot). And that overhead might not be deemed acceptable? If people think it's worth pursuing, then I should get a chance to clean it up a bit the week after next. |
Hi @hynek, great to see that version
24.1.0
is finally out, have been looking forward to it for quite a while 👍🏼 🙃One reason why I was waiting for it is because I wanted to enable slots for our classes that rely on
cached_property
. After upgrading to24.1.0
and activating slots, everything seemed to work fine ... except for our docs 😕We build them with sphinx and the error I'm getting indicates trouble coming from exactly the
cached_property
part. Here is an excerpt from the critical classfor which I get the following error during our sphinx built
I haven't yet tried to pull together a minimal self-contained example, because I first wanted to ask if you already have an idea what could cause the problem – perhaps the
cached_property
integration is simply not yet correct/complete?All I can say for now is:
cached_property
into a regularproperty
so the issue really does seem to be related to the latests
attrs
changes.Let me know if you need more information!
The text was updated successfully, but these errors were encountered: