Skip to content

Requiring field annotations even though type can be inferred from field type #68

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

Closed
pirate opened this issue Apr 18, 2019 · 37 comments · Fixed by #177 or #182
Closed

Requiring field annotations even though type can be inferred from field type #68

pirate opened this issue Apr 18, 2019 · 37 comments · Fixed by #177 or #182
Labels
bug Something isn't working
Milestone

Comments

@pirate
Copy link
Contributor

pirate commented Apr 18, 2019

Right now mypy is complaining that my fields aren't annotated with types.

Screen Shot 2019-04-18 at 8 11 05 AM

Is this expected behavior? If so, why? Cant the type be inferred from the field value itself?

Thanks!

@mkurnikov
Copy link
Member

Did you set plugins = mypy_django_plugin.main in your mypy.ini?

@pirate
Copy link
Contributor Author

pirate commented Apr 18, 2019

Yup (and other Django type hints seem to be working). Here are my two config files:

mypy.ini:

[mypy]
plugins =
    mypy_django_plugin.main

mypy_django.ini:

[mypy_django_plugin]
django_settings = penny.settings
ignore_missing_settings = True
ignore_missing_model_attributes = True

@pirate
Copy link
Contributor Author

pirate commented Jun 14, 2019

Bump, have any ideas? Is this expected behavior? If not, where can we look to solve it? Thanks!

@ncvc
Copy link

ncvc commented Aug 15, 2019

I'm also seeing this issue with

django = "=2.2.4"
mypy = "=0.720"
django-stubs = "=1.0.2"

Has anyone found a workaround for this?

@pirate
Copy link
Contributor Author

pirate commented Aug 15, 2019

We just added manual type annotation comments to all the fields, never found a solution unfortunately.

@sobolevn
Copy link
Member

@pirate have you tried 1.0.x?

@pirate
Copy link
Contributor Author

pirate commented Aug 15, 2019

Yup, still happening on 1.0.2:

Pipfile.lock:

"django-stubs": {
            "hashes": [
                "sha256:5018c38542ac1de55ab1782981b1c3eb690e7515a216efc44a9c01862b8cf8b5",
                "sha256:775a3fe5939f2813c6e158b2ea0f3903c91496233e3f2c12384e78915efdf8d8"
            ],
            "index": "pypi",
            "version": "==1.0.2"
        },

mypy.ini:

[mypy]
plugins =
    mypy_django_plugin.main

mypy_django.ini

[mypy_django_plugin]
django_settings = core.settings
ignore_missing_settings = True
ignore_missing_model_attributes = True

image

@aleksanb
Copy link
Contributor

What IDE are you using? If you invoke mypy from the command line, does it work then?
I har trouble getting it to work in VSCode at some point as it wouldn't pick up my config file correctly based on which folder was root in my VSCode project.

If you manage to run it correctly and types are revealed with reveal_type in the cli, then it's your IDE setup that isn't working.

@ncvc
Copy link

ncvc commented Aug 16, 2019

This issue occurs for me outside of my IDE, running mypy . on the command line.

@pirate
Copy link
Contributor Author

pirate commented Aug 16, 2019

Confirmed here as well, it's not limited to the IDE. All other django stubs are working fine too, it's just the model Fields that are having this problem.

@ntietz
Copy link

ntietz commented Aug 26, 2019

I've run into this as well. When I ran into it, I noticed that it was occurring anywhere I was using django_extensions.db.models.TimeStampedModel as my parent class, rather than models.Model. When I switched to using TimeStampedModel as a mixin, like this, it worked as expected:

class FooModel(TimeStampedModel, models.Model):
    pass

@pirate @ncvc either of you using django_extensions? If so, that might be the common link.

@pintofcat
Copy link

Got the same error at 1.0.2

@pirate
Copy link
Contributor Author

pirate commented Sep 3, 2019

We do happen to be using django_extensions in our codebase, but I don't think it's the root because we don't use django_extensions.db.models.TimeStampedModel or any other django_extensions base model in our code.

We have a different custom base model that we use for everything class BaseModel(models.Model): ....
So maybe it's a bug with it not being able to infer field value types on custom base-models in general.

@sobolevn sobolevn added the bug Something isn't working label Sep 3, 2019
@Tatsh
Copy link
Contributor

Tatsh commented Sep 13, 2019

@ntietz' solution seems to work for those using django_extensions.

@benrudolph
Copy link

Just confirming that this didn't have anything to do with django_extensions, but rather the fact that I was inheriting from a base model that inherited from models.Model (exactly @pirate 's issue)

@Tatsh
Copy link
Contributor

Tatsh commented Sep 16, 2019

It would be good to have this check every class defined in a typical models.py (or models/*.py) and then inspect the inherited class to check if it extends Django's Model class. I have not yet encountered any side issues with @ntietz' solution but there could be some.

@benrudolph
Copy link

benrudolph commented Sep 16, 2019

For me this actually results in a type error with the Meta class: Definition of "Meta" in base class "BaseModel" is incompatible with definition in base class "Model". Since our BaseModel defines a Meta (abstract = True)

@mkurnikov
Copy link
Member

@Tatsh

It would be good to have this check every class defined in a typical models.py (or models/*.py) and then inspect the inherited class to check if it extends Django's Model class. I have not yet encountered any side issues with @ntietz' solution but there could be some.

So, like,

class ModelMixin:
    name = models.CharField(max_length=100)

class MyModel(models.Model, ModelMixin):
    pass

look at MyModel, see all the bases, and for every base, process Field subclass as if it was a model. Like this? Seems like a good idea. I'll try to work on it today or tomorrow.

@Tatsh
Copy link
Contributor

Tatsh commented Sep 16, 2019

You only have to make sure the last class passed to extend from in that example, at some point, extends models.Model.

In Python, the last class passed takes precedence over the rest which should only be mixins.

@mkurnikov
Copy link
Member

mkurnikov commented Sep 17, 2019

I've added processing of Field instantiation for mixin classes in #167, but after more thorough reading of the issue, it doesn't seem to be all that needed to be done.
Could anyone who experience this issue currently, use PR branch and if it doesn't work - create failing test in the same format as done in
https://github.com/typeddjango/django-stubs/tree/master/test-data/typecheck
?
It would be much easier to provide a fix with a failing test.

@benrudolph
Copy link

#172

@benrudolph
Copy link

Nearly all my errors go away when i don't inherit from from django.contrib.gis.db import models

@mkurnikov
Copy link
Member

I never worked with django.contrib.gis, but it seems that there's another base models.Model there which I didn't considered before.
Thanks for the insight, I'll try to find time for it this weekend.

@benrudolph
Copy link

thanks @mkurnikov let me know if i can help

@pirate
Copy link
Contributor Author

pirate commented Sep 23, 2019

@mkurnikov that fix in #177 looks GIS-specific, I don't think it will fix the general case of Fields on a model that inherits from another custom BaseModel.

@Tatsh
Copy link
Contributor

Tatsh commented Sep 23, 2019

That PR doesn't solve the issue for those using django_extension classes like TimestampedModel. This issue should be kept open.

@mkurnikov
Copy link
Member

mkurnikov commented Sep 23, 2019

I think the idea is like that: if you have a base model which is type annotated (mypy knows that it's a models.Model subclass) it will correctly infer is as a model, and everything should be ok.

If it's not, then mypy will infer Any, and nothing will work. It's just a guess though, could you confirm it with reveal_type(YourBaseModel) somewhere in the code (under if TYPE_CHECKING)?

UPD.
If my guess is correct,

if TYPE_CHECKING:
    class TimestampedModel(TimestampedModel, models.Model):
        pass

should solve the issue. On runtime this clause will never be executed, but mypy will know that it's a models.Model subclass. Could you try?

@mkurnikov mkurnikov reopened this Sep 23, 2019
@qcoumes
Copy link

qcoumes commented Sep 23, 2019

Having the same problem using another ModelBase (MPTTModel from django-mptt), I can confirm this:

if TYPE_CHECKING:
    reveal_type(MPTTModel)

class Resource(MPTTModel):
    author = models.ForeignKey(User, on_delete=models.SET_NULL, null=True)
    short_description = models.CharField(max_length=255)
    long_description = models.TextField(default="")
    dependencies = models.ManyToManyField("self")
    metadata = models.ForeignKey(Metadata, on_delete=models.PROTECT)
    file = models.PositiveIntegerField(default=file_auto_increment, db_index=True)
    creation = models.DateTimeField(auto_now_add=True)
    previous = TreeForeignKey("self", on_delete=models.PROTECT, null=True, related_name=")

I got :

note: Revealed type is 'Any'
error: Need type annotation for 'author'
error: Need type annotation for 'short_description'
error: Need type annotation for 'long_description'
error: Need type annotation for 'dependencies'
error: Need type annotation for 'metadata'
error: Need type annotation for 'file'
error: Need type annotation for 'creation'

And in my case,

if TYPE_CHECKING:
    class MPTTModel(MPTTModel, models.Model):
        pass

Did not solve the problem:

Name 'MPTTModel' already defined (possibly by an import)
error: Need type annotation for 'author'
error: Need type annotation for 'short_description'
error: Need type annotation for 'long_description'
error: Need type annotation for 'dependencies'
error: Need type annotation for 'metadata'
error: Need type annotation for 'file'
error: Need type annotation for 'creation'

Using:

if TYPE_CHECKING:
    class MPTTModel(models.Model):
        pass
else:
    from mptt.models import MPTTModel

Solved the problem of attribute annotation, but I obviously got complaints about method declared in MPTTModel :

error: "Resource" has no attribute "get_level"
error: "Resource" has no attribute "get_ancestors"; maybe "ancestors"?
error: "Resource" has no attribute "version"
[...]

@mkurnikov
Copy link
Member

Would this work?

if TYPE_CHECKING:
    from mptt.models import MPTTModel as _MPTTModel

    class MPTTModel(_MPTTModel, models.Model):
        pass
else:
    from mptt.models import MPTTModel

(just tried it locally, seems fine)

@qcoumes
Copy link

qcoumes commented Sep 23, 2019

This one did work for me, thanks !

@pirate
Copy link
Contributor Author

pirate commented Sep 23, 2019

 if TYPE_CHECKING:
     from third_party.models import BaseModel as _BaseModel

     class BaseModel(_BaseModel, models.Model):
         pass
 else:
     from third_party.models import BaseModel

This is a bit of a gnarly a hack to require adding at the top of many files all over a large codebase. Is there no generic solution to fix this for all base models?

>>> from core.models import Version   # our model that inherits from a custom BaseModel
>>> from django.db import models
>>> issubclass(Version, models.Model)
True

Given that they're still models.Model subclass instances, is there no mypy-compatible way for django-stubs to check farther up the inheritance chain internally and see whether the Class derives from models.Model?

@mkurnikov
Copy link
Member

What's a result of

if TYPE_CHECKING:
    reveal_type(Version)

in your case?

@pirate
Copy link
Contributor Author

pirate commented Sep 24, 2019

sync/models.py:797: note: Revealed type is 'def (*args: Any, **kwargs: Any) -> sync.models.Version'

@mkurnikov mkurnikov added this to the 1.2.0 milestone Sep 24, 2019
@pirate
Copy link
Contributor Author

pirate commented Sep 24, 2019

For anyone following this, @mkurnikov's new solution worked for me! #182

It looks like it's properly checking all of the model base classes in the inheritance chain now, and I no longer get Field Need type annotation warnings on models that inherit from custom BaseModels.

Thanks for the quick turnaround time on that fix over the last week @mkurnikov!

@Tatsh
Copy link
Contributor

Tatsh commented Sep 25, 2019

Will that PR get merged? Is there going to be a release soon?

@pirate
Copy link
Contributor Author

pirate commented Sep 25, 2019

@Tatsh if you need it asap you can install it from the branch:

pip install -e 'git+https://github.com/mkurnikov/django-stubs@custom-model-base#egg=django-stubs'
# or
pipenv install ...

@mkurnikov
Copy link
Member

mkurnikov commented Sep 25, 2019

@Tatsh Release is planned a week or two after 0.730 is released. As for merge, I don't know, I'll try to make it till the end of the week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment