-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
mypy: fix mypy errors in wladmin module. Rel: #3703 #12690
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12690 +/- ##
==========================================
+ Coverage 91.15% 91.19% +0.03%
==========================================
Files 596 596
Lines 61027 61031 +4
Branches 9660 6334 -3326
==========================================
+ Hits 55629 55657 +28
- Misses 3727 3728 +1
+ Partials 1671 1646 -25
|
weblate/wladmin/sites.py
Outdated
@@ -12,7 +12,7 @@ | |||
from django.utils.translation import gettext, gettext_lazy | |||
|
|||
if TYPE_CHECKING: | |||
from weblate.auth.models import AuthenticatedHttpRequest | |||
from django.http import HttpRequest |
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.
The intention here is that Django's HttpRequest
can have AnonymousUser
what can't happen in Weblate. That's why AuthenticatedHttpRequest
was introduced. Without that each access to reuqest.user
has complains from 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.
Actually, this is an upstream recommended approach: https://github.com/typeddjango/django-stubs?tab=readme-ov-file#how-can-i-create-a-httprequest-thats-guaranteed-to-have-an-authenticated-user
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.
Yes, I've seen that answer in FAQ. The reason of this change is because of a method each_context
, that has a signature with HttpRequest
. The mypy error is:
error: Argument 1 of "each_context" is incompatible with supertype "AdminSite"; supertype defines the argument type as "HttpRequest" [override]
note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
note: This violates the Liskov substitution principle
For now I have two solutions on my mind, cast
and ignore
.
cast("AuthenticatedHttpRequest", request)
doesn't make much sense, because the function doesn't actually need arequest.user
# type: ignore[override]
doesn't seem that bad actually
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 was using ignore for override so far, IMHO it is the best approach
Merged, thanks for your contribution! |
Proposed changes
Fixes (7) mypy errors in wladmin module.
Checklist
Other information
list of fixed errors