-
-
Notifications
You must be signed in to change notification settings - Fork 460
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
Fix @classproperty decorator #1287
Conversation
Removed unnecessary `self: Self` hints on classproperty methods.
django-stubs/utils/functional.pyi
Outdated
def __init__(self: Self, method: Callable[[Self], _Get] | None = ...) -> None: ... | ||
def __get__(self: Self, instance: Self | None, cls: type[Self] = ...) -> _Get: ... | ||
def getter(self: Self, method: Callable[[Self], _Get]) -> classproperty[_Get]: ... | ||
def __init__(self, method: Callable[[Self], _Get] | None = ...) -> None: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now Self
seems unbound. It can be Any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually understand how this mechanism works 😆 I just reverted the unwanted changes and verified via the test that it works.
If I add self: Self
here, it no longer works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy binds variables in two cases:
- If they are in
Generic[]
definition, like we have here with_Get
- If it is used more than once in a function:
def some(a: A) -> A: return a
Here Self
is unbound and should be changed to Any
with a comment, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it could be done like in python/typeshed stdlib/builtins.pyi classmethod
_R_co = TypeVar("_R_co", covariant=True)
...
class classmethod(Generic[_R_co]):
@property
def __func__(self) -> Callable[..., _R_co]: ...
@property
def __isabstractmethod__(self) -> bool: ...
def __init__(self: classmethod[_R_co], __f: Callable[..., _R_co]) -> None: ...
def __get__(self, __obj: _T, __type: type[_T] | None = ...) -> Callable[..., _R_co]: ...
if sys.version_info >= (3, 10):
__name__: str
__qualname__: str
@property
def __wrapped__(self) -> Callable[..., _R_co]: ...
PEP 673 - Self Type describes Self
as shorthand for: TClassName = TypeVar("TClassName", bound="ClassName")
. So maybe covariant TypeVar would be the fix here?
Also PEP 673 was only just introducted in python 3.11. Maybe that could be a problem too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then again djangos classproperty is implemented a bit differently than CPythons classmethod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw that _Get
already is the covariant TypeVar, so that should be fine.
Looking at the typehints for @property I think I now understand what @sobolevn means (simply replace the remaining Self
with Any
):
fget: Callable[[Any], _Get] | None
def __init__(self, method: Callable[[Any], _Get] | None = ...) -> None: ...
def __get__(self, instance: Any | None, cls: type[Any] = ...) -> _Get: ...
def getter(self, method: Callable[[Any], _Get]) -> classproperty[_Get]: ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed works just as well with Any
. I've pushed this change.
LGTM and the fix works locally for me :) |
Removed unnecessary
Self
hints on classproperty methods.Related issues
The removal of
self: Self
fixes #1285. Regressed in #1253.