-
Notifications
You must be signed in to change notification settings - Fork 662
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
Improve type of Token.for_user
to allow subclasses
#776
Conversation
@@ -197,7 +197,7 @@ def check_exp( | |||
raise TokenError(format_lazy(_("Token '{}' claim has expired"), claim)) | |||
|
|||
@classmethod | |||
def for_user(cls, user: AuthUser) -> "Token": | |||
def for_user(cls: Type[T], user: AuthUser) -> T: |
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.
Hello @sterliakov,
Thank you for this patch. This approach demonstrate a forward-thinking approach. In this case, modifying the "Token"
user to generic type [T]
address the issue in a clear concise way since, a similar pattern had already appeared in the codebase.
While there is no breaking changes at the moment, would you be interested to write tests for this functionality focusing on its impact on the Token
class itself and the subclass, RefreshToken
?
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.
Hi! I'd be interested in contributing to type checking of this project in Jan.
There are other typing issues I spotted (e.g. TokenObtainSerializer
should be generic in token type to have correct get_token
return type), and I have a typed codebase (not OSS, unfortunately) where many components of this package are extended (so some issues will pop up when I upgrade).
Regarding the tests - it would be not as straightforward, unfortunately, due to django-stubs
complicated nature that requires code import and execution. Some aspects can be tested trivially, though, and I am willing to cover such patterns. Typechecking tests are usually of shape "here's the file, run mypy
on it and check output" (you can find examples e.g. here).
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.
Ough. I failed to notice this initially, but I also want to add mypy
to CI very much: types seem to be not checked at all now.
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 would re-review after the holidays! Happy new year in advance @sterliakov 🌟
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.
Hi @sterliakov, reopening discussion on this patch, we could work with mypy
on a separate MR
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.
LGTM!
The same approach is already used in
BlacklistMixin
. The issue with existingToken
return type can be demonstrated by