Add special support for @django.cached_property needed in django-stubs
#18959
Conversation
| if _is_django_cached_property(runtime): | ||
| yield from _verify_final_method(stub.func, runtime.func, MISSING) | ||
| return | ||
| if inspect.isdatadescriptor(runtime): | ||
| # It's enough like a property... | ||
| return |
There was a problem hiding this comment.
this method is called _verify_readonly_property, but inspect.isdatadescriptor() only returns True if an object has a __set__ method (and descriptors with __set__ methods are analogous to read/write properties rather than read-only properties). Could we avoid adding special-casing for django here if we just checked whether runtime is an object with a __get__ method rather than checking if the object is a data descriptor?
Something like this? (Though you probably want to use getattr_static or something since the getattr() call here might raise TypeError or something crazy)
| if _is_django_cached_property(runtime): | |
| yield from _verify_final_method(stub.func, runtime.func, MISSING) | |
| return | |
| if inspect.isdatadescriptor(runtime): | |
| # It's enough like a property... | |
| return | |
| if callable(getattr(type(runtime), "__get__", None)): | |
| # It's enough like a property... | |
| return |
There was a problem hiding this comment.
This way I can't verify that stub.func and runtime.func are the same :(
But, this extra check can be added on its own in a separate PR.
| # https://github.com/typeddjango/django-stubs | ||
| try: | ||
| from django.utils.functional import cached_property # type: ignore[import-not-found] | ||
| except Exception: |
There was a problem hiding this comment.
Should this be except Exception and not except ImportError?
Hi!
We, in
django-stubs, have a lot of usages of@cached_propertydecorator that is a part ofdjango: https://docs.djangoproject.com/en/5.2/ref/utils/#django.utils.functional.cached_propertyAll usages of it we have to add to
subtest/allowlist.txt, which is not great. In typing we reuse@functools.cached_propertyto have all the benefits of its inference: https://github.com/typeddjango/django-stubs/blob/ee8e8b11c37866969ff0406be20591a067dfa983/django-stubs/utils/functional.pyi#L3-L4But,
stubtestis not happy with this move: because in runtime objects havedjango.utils.functional.cached_propertytype and we see the following error:So, we add all
@django.utils.functional.cached_propertyusages to ourallowlist.txt. There are LOTS of entries there: https://github.com/typeddjango/django-stubs/blob/ee8e8b11c37866969ff0406be20591a067dfa983/scripts/stubtest/allowlist.txt#L158-L425Moreover, we have to always tell about this problem to new contributors on review :(
That's why I propose to special case this as we do with other
property-likes.I've tested locally and it works perfectly. I don't want to complicate the CI with
djangoinstallation and special tests. So, I added# pragma: no coverto indicate that it is not tested.