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.

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 type annotations for 3rd party models #2440

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions mypy_django_plugin/lib/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,10 @@ def is_model_type(info: TypeInfo) -> bool:
return info.metaclass_type is not None and info.metaclass_type.type.has_base(fullnames.MODEL_METACLASS_FULLNAME)


def is_registered_model_type(info: TypeInfo, django_context: "DjangoContext") -> bool:
return info.fullname in {get_class_fullname(cls) for cls in django_context.all_registered_model_classes}


def get_model_from_expression(
expr: Expression,
*,
Expand Down
5 changes: 4 additions & 1 deletion mypy_django_plugin/transformers/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,10 @@ def transform_into_proper_return_type(ctx: FunctionContext, django_context: Djan
assert isinstance(default_return_type, Instance)

outer_model_info = helpers.get_typechecker_api(ctx).scope.active_class()
if outer_model_info is None or not helpers.is_model_type(outer_model_info):
if outer_model_info is None or (
not helpers.is_model_type(outer_model_info)
and not helpers.is_registered_model_type(outer_model_info, django_context)
):
return ctx.default_return_type

assert isinstance(outer_model_info, TypeInfo)
Expand Down
3 changes: 3 additions & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ Django==5.1.3; python_version >= '3.10'
-e ./ext
-e .[redis,compatible-mypy,oracle]

# 3rd Party Library for Testing
django-extensions==3.2.3

# Overrides:
mypy==1.13.0
pyright==1.1.388
15 changes: 15 additions & 0 deletions tests/typecheck/models/test_3rd_party_models.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
- case: handles_type_annotations_on_3rd_party_models
installed_apps:
- django_extensions
- myapp
main: |
from myapp.models import A
Copy link
Member

Choose a reason for hiding this comment

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

please, add a reveal_type to A().name

files:
- path: myapp/__init__.py
- path: myapp/models.py
content: |
from django.db import models
from django_extensions.db.models import TimeStampedModel # type: ignore[import-untyped]
Copy link
Member

@flaeppe flaeppe Nov 10, 2024

Choose a reason for hiding this comment

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

I actually disagree with the changes here. The main issue in #2341 is that django-extensions is a package that doesn't export any type hints.

The ignore comment I'm highlighting here is saying precisely that. What is experienced in #2341 is how mypy behaves when there are no type hints.

What is attempted to be repaired here is only the first line of issues, there could be an arbitrary amount of other problems to solve after this one, coming from it being an untyped package.

There are multiple other approaches to resolve this, that integrates better with the type checker. e.g.

  • Add and export type hints in django-extensions
  • Create a stubs package(.pyi) just like django-stubs for django-extrnsions
  • Add stubs (.pyi-files) for django-extensions to your project, locally
  • Perhaps django-stubs would consider including stubs in a similar fashion to typeshed, that includes/maintains a bunch of 3rd party stubs?

Regardless, I think merging the changes here is a step in the wrong direction as it's outside of django-stubs domain


class A(TimeStampedModel):
name = models.CharField()
Loading