-
-
Notifications
You must be signed in to change notification settings - Fork 454
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
Disallow Any
generics
#1339
Disallow Any
generics
#1339
Conversation
…istian/fix-action-return-type
Co-authored-by: Marti Raudsepp <[email protected]>
Co-authored-by: Marti Raudsepp <[email protected]>
# This is deliberately different from _DisplayT defined in contrib.admin.options | ||
_DisplayCallable: TypeAlias = Union[Callable[[_ModelAdmin, _Model], Any], Callable[[_Model], Any]] # noqa: Y037 | ||
_DisplayCallableT = TypeVar("_DisplayCallableT", bound=_DisplayCallable) | ||
_DisplayCallableT = TypeVar("_DisplayCallableT", bound=_DisplayCallable[ModelAdmin[Model], Model]) | ||
_ActionReturn = TypeVar("_ActionReturn", bound=HttpResponseBase | 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.
Note: this currently builds off of my other branch, #1331.
warn_unreachable = True | ||
disallow_untyped_defs = true | ||
disallow_incomplete_defs = true | ||
disallow_any_generics = false |
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.
What's the right way to do this? I want disallow_any_generics = true
in our mypy.ini
, but I'd like to disable it in tests since I don't want to touch every test. Solutions that came to mind:
- Rewrite all the tests (😭)
- Add a
mypy_config
declaration to all failing tests - Disable for the
mypy-myapp.*
module inmypy.ini
-- I couldn't get this working. - Worst possible solution: copy and paste
mypy.ini
tomypy-test.ini
for this proof of concept.
self, request: HttpRequest, callback: Callable | None, callback_args: tuple, callback_kwargs: dict[str, Any] | ||
self, | ||
request: HttpRequest, | ||
callback: Callable[..., Any] | 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.
My goal was to match the existing type unless there was an obvious improvement. For example, when replacing Callable
I'd use reveal_type()
to ensure I'm matching it 1:1. I tried very hard to avoid adding Any
unless that's truly what was previously being inferred.
@@ -12,7 +12,7 @@ | |||
from django.db.models.query import QuerySet | |||
from django.utils.translation import gettext_lazy as _ | |||
|
|||
def an_action(modeladmin: "A", request: HttpRequest, queryset: QuerySet) -> None: | |||
def an_action(modeladmin: "A", request: HttpRequest, queryset: QuerySet[Model]) -> 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.
I recommend using some real model in tests, like contrib.auth.models.User
, to be more similar to actual usage in the field.
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.
Oops, I don't think I'd meant to commit this. IIRC there's a test failure in both this file and test_decorators
, at least partially because of the display()
fixes, but I appreciate the suggestion and I'll do that when resolving the failing tests.
page_kwarg: str | ||
ordering: str | Sequence[str] | None | ||
def get_queryset(self) -> _SupportsPagination[_M]: ... | ||
def get_ordering(self) -> str | Sequence[str] | None: ... | ||
def paginate_queryset( | ||
self, queryset: _SupportsPagination[_M], page_size: int | ||
) -> tuple[Paginator, Page, _SupportsPagination[_M], bool]: ... | ||
) -> tuple[Paginator[_M], Page[_M], _SupportsPagination[_M], bool]: ... |
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.
In a few cases, I took a leap of faith and decided to Paginator[_M]
rather than Paginator[Model]
.
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.
Yep these really improve our typing accuracy, thanks!
Note that I just merged #1260, which had changes in mypy.ini. Sorry for the conflicts. |
…istian/disallow-any-generics
No problem at all, thanks for letting me know so I can resolve the conflicts real quick. |
I think this change is desirable and will improve the quality of our stubs. There are two new conflicts in |
I tried this change with my real codebase and it causes significant false positives. Not sure why this one appears with
These seem to be from changing
Maybe you should revert back to |
Problem: After this thread I realized that I've been taking
disallow_any_generics
for granted in my other projects, and realized that without it enabled in django-stubs it's very easy to make silly mistakes and much harder to notice when accidentally passingAny
.Solution: I tried enabling
disallow_any_generics
and then just slowly worked through each type error.Related issues
Maybe closes #64? I'm not sure.