Skip to content
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.

Sign up for GitHub

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 regression in '.bulk_update()' annotations for Django 3.2 #1114

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix regression in '.bulk_update()' annotations for Django 3.2 #1114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion django-stubs/db/models/manager.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class BaseManager(Generic[_T]):
def bulk_create(
self, objs: Iterable[_T], batch_size: Optional[int] = ..., ignore_conflicts: bool = ...
) -> List[_T]: ...
def bulk_update(self, objs: Iterable[_T], fields: Sequence[str], batch_size: Optional[int] = ...) -> int: ...
def bulk_update(self, objs: Iterable[_T], fields: Iterable[str], batch_size: Optional[int] = ...) -> Optional[int]: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By changing to Optional[int] we've gone from having a correct return type for more recent versions to have an incorrect return type for all versions (don't think bulk_update has ever returned Optional[int]).

Could it work to wrap the bulk_update definition in an if-statement that branches on the django version instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great idea, and actually what I attempted first, but I could not get it to work, and I'm having a hard time finding any examples on the internet that show that conditional type hinting logic based on the installed Django version is indeed possible. For example, I don't see any other example of that kind of logic occurring in this codebase, and, from what I understand, that has historically been the reason why each version of django-stubs has only ever claimed/attempted to support a single version of Django - because, AFAIK, mypy is specifically only able to handle conditional logic in a few limited cases:

  • sys.version_info
  • sys.platform
  • typing.TYPE_CHECKING
  • an in-scope variable named MYPY

(Source: https://mypy.readthedocs.io/en/stable/common_issues.html#python-version-and-system-platform-checks)

But if you want it to handle conditional logic based on anything else, such as the installed Django version, you're out of luck (from what I understand).

For example, mypy is fine with this (which isn't what we want, but just showing an example of how it can handle conditional logic based on one of the above things mypy explicitly says it can handle):

import sys

if sys.version_info < (4,):
    def bulk_update(self, objs: Iterable[_T], fields: Iterable[str], batch_size: Optional[int] = ...) -> None: ...
else:
    def bulk_update(self, objs: Iterable[_T], fields: Iterable[str], batch_size: Optional[int] = ...) -> int: ...

Results in no mypy error.

But, if instead we do what we actually want to do, which is have a conditional on the Django version, not the Python version, mypy won't accept it:

import django

if django.VERSION < (4,):
    def bulk_update(self, objs: Iterable[_T], fields: Iterable[str], batch_size: Optional[int] = ...) -> None: ...
else:
    def bulk_update(self, objs: Iterable[_T], fields: Iterable[str], batch_size: Optional[int] = ...) -> int: ...

We get this mypy error: error: All conditional function variants must have identical signatures

I've tried all sorts of combinations with the things mypy allows conditional logic on with no luck:

  • if sys.version_info and django.VERSION < (4,):
  • if sys.platform and django.VERSION < (4,):
  • if typing.TYPE_CHECKING and django.VERSION < (4,):
  • MYPY = True
if MYPY and django.VERSION < (4,):

All result in the same mypy error: error: All conditional function variants must have identical signatures

I've also tried nesting the conditionals for each of the above cases too, in the hopes that it would somehow allow mypy to process everything under the conditional - for example:

if typing.TYPE_CHECKING:
    if django.VERSION < (4,):
        def bulk_update(self, objs: Iterable[_T], fields: Iterable[str], batch_size: Optional[int] = ...) -> None: ...
    else:
        def bulk_update(self, objs: Iterable[_T], fields: Iterable[str], batch_size: Optional[int] = ...) -> int: ...

But, again, same error each time: error: All conditional function variants must have identical signatures

I'm not having much luck in finding any information about this elsewhere (StackOverflow, Google, etc.) where someone has used mypy successfully in handling conditional logic outside of the 4 special cases it specifically supports, unfortunately. Hopefully someone more knowledgeable than me can come along and show me what I'm missing on how this can be possible 🙃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This feels somewhat of a limitation that could be resolved through mypy. But I'm not at all sure.

What I'm thinking is that if mypy exposes some sort of hook for plugins to declare these sort of constants by themselves.

Until we can find support for declaring 2 versions of the bulk_update signature I'd lean towards having django-stubs exporting types for newer versions of Django. But perhaps that's just me.

def get_or_create(self, defaults: Optional[MutableMapping[str, Any]] = ..., **kwargs: Any) -> Tuple[_T, bool]: ...
def update_or_create(
self, defaults: Optional[MutableMapping[str, Any]] = ..., **kwargs: Any
Expand Down
2 changes: 1 addition & 1 deletion django-stubs/db/models/query.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class _QuerySet(Generic[_T, _Row], Collection[_Row], Reversible[_Row], Sized):
def bulk_create(
self, objs: Iterable[_T], batch_size: Optional[int] = ..., ignore_conflicts: bool = ...
) -> List[_T]: ...
def bulk_update(self, objs: Iterable[_T], fields: Iterable[str], batch_size: Optional[int] = ...) -> int: ...
def bulk_update(self, objs: Iterable[_T], fields: Iterable[str], batch_size: Optional[int] = ...) -> Optional[int]: ...
def get_or_create(self, defaults: Optional[MutableMapping[str, Any]] = ..., **kwargs: Any) -> Tuple[_T, bool]: ...
def update_or_create(
self, defaults: Optional[MutableMapping[str, Any]] = ..., **kwargs: Any
Expand Down