-
-
Notifications
You must be signed in to change notification settings - Fork 493
create and re-use TypeAlias
es and TypeVar
s for "user" and "any user"
#2384
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,9 +12,22 @@ from django.db.models.manager import EmptyManager | |
from django.utils.functional import _StrOrPromise | ||
from typing_extensions import Self | ||
|
||
_AnyUser: TypeAlias = Model | AnonymousUser | ||
# This is our "placeholder" type the mypy plugin refines to configured 'AUTH_USER_MODEL' | ||
# wherever it is used as a type. The most recognised example of this is (probably) | ||
# `HttpRequest.user` | ||
_User: TypeAlias = AbstractBaseUser | ||
|
||
def update_last_login(sender: type[AbstractBaseUser], user: AbstractBaseUser, **kwargs: Any) -> None: ... | ||
_UserModel: TypeAlias = type[_User] | ||
|
||
_AnyUser: TypeAlias = _User | AnonymousUser | ||
terencehonles marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# These are only needed for generic classes in order to bind to a specific implementation | ||
_AnyUserType = TypeVar("_AnyUserType", bound=_AnyUser) # noqa: PYI018 | ||
|
||
# do not use the alias `_User` so the bound remains at `AbstractUser` | ||
_UserType = TypeVar("_UserType", bound=AbstractUser) | ||
Comment on lines
+27
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this bound seems incorrect -- or at least it breaks the usage in sentry the docs for django seem to indicate AbstractBaseUser as the bound here ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this probably is more correct. thanks for noticing! btw, can we test compat with sentry somehow? like running it should be possible if projects that you are talking about are opensource. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep yep -- the repo in question is fully open source https://github.com/getsentry/sentry there's a few problems that might be showstoppers though:
so I guess as long as it doesn't have to typecheck completely cleanly it should be fine? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. created #2645 for the fix |
||
|
||
def update_last_login(sender: _UserModel, user: _User, **kwargs: Any) -> None: ... | ||
|
||
class PermissionManager(models.Manager[Permission]): | ||
def get_by_natural_key(self, codename: str, app_label: str, model: str) -> Permission: ... | ||
|
@@ -38,23 +51,21 @@ class Group(models.Model): | |
permissions = models.ManyToManyField(Permission) | ||
def natural_key(self) -> tuple[str]: ... | ||
|
||
_T = TypeVar("_T", bound=Model) | ||
|
||
class UserManager(BaseUserManager[_T]): | ||
class UserManager(BaseUserManager[_UserType]): | ||
def create_user( | ||
self, username: str, email: str | None = ..., password: str | None = ..., **extra_fields: Any | ||
) -> _T: ... | ||
) -> _UserType: ... | ||
def create_superuser( | ||
self, username: str, email: str | None = ..., password: str | None = ..., **extra_fields: Any | ||
) -> _T: ... | ||
) -> _UserType: ... | ||
def with_perm( | ||
self, | ||
perm: str | Permission, | ||
is_active: bool = ..., | ||
include_superusers: bool = ..., | ||
backend: str | None = ..., | ||
obj: Model | None = ..., | ||
) -> QuerySet[_T]: ... | ||
) -> QuerySet[_UserType]: ... | ||
|
||
class PermissionsMixin(models.Model): | ||
is_superuser = models.BooleanField() | ||
|
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.
This is not generic at runtime, which prevents this from being subclassed.
mypy complains:
error: Missing type parameters for generic type "SetPasswordForm" [type-arg]
Fails at runtime:
TypeError: 'DeclarativeFieldsMetaclass' object is not subscriptable
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.
@andersk can you please send a PR to
ext/
? So we can monkeypatch this object in runtime.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 we should add a small test to ensure that ? Something parsing pyi file for ``generic` calls and ensuring they match the one declared in the ext ? I can try something like that
Uh oh!
There was an error while loading. Please reload this page.
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.
Sure, this would be a great addition. I have no idea on how to actually do that :)
Maybe a
stubtest
extra check? 🤔I can open an issue in mypy.
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.
Would be nice if stubtest can catch that indeed
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.
python/mypy#19032