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

django-stubs-ext: Add BaseModelMeta for typing Model inner Meta class #1375

Merged
merged 6 commits into from
Apr 8, 2023

Conversation

intgr
Copy link
Collaborator

@intgr intgr commented Feb 22, 2023

I've had some thoughts about how to expand django-stubs-ext.

We can provide a base class for the Model inner class Meta. This inner class provides type hints for all available attributes, and thus mypy can check whether actual usages have valid types or not.

Example usage:

from django.db import models
from django_stubs_ext.db.models import BaseModelMeta

class MyModel(models.Model):
    example = models.CharField(max_length=100)

    class Meta(BaseModelMeta):
        #     ^^^^^^^^^^^^^^^ This needs to be added
        ordering = ["example"]
        unique_together = {}  # E: Incompatible types in assignment (expression has type "Dict[<nothing>, <nothing>]", base class "BaseModelMeta" defined the type as "Union[Sequence[Sequence[str]], Sequence[str]]")

@intgr intgr changed the title django-stubs-ext: Add BaseModelMeta for typing Model class Meta inner class django-stubs-ext: Add BaseModelMeta for typing Model inner Meta class Feb 22, 2023
@flaeppe
Copy link
Member

flaeppe commented Feb 22, 2023

Couldn't this be plugin territory also?

Since it's Django's metaclass extension that constructs the Option instance. I suppose one would've hidden the inheritance/declaration behind a if TYPE_CHECKING if types were declared in Django.

Perhaps that should be the suggestion/example, instead of suggesting runtime?

if TYPE_CHECKING:
    class Meta(BaseModelMeta):
        ...

Also, a different thought as I'm writing. MyModel.Meta is never mutated like this suggests, right? Isn't it ported to the MyModel._meta attribute, which is an Option instance?

@intgr
Copy link
Collaborator Author

intgr commented Feb 23, 2023

Couldn't this be plugin territory also?

True, I suppose that would be better. But I'm afraid the implementation will end up being more complex than it sounds. I'm also not very familiar with plugin code.

I suppose one would've hidden the inheritance/declaration behind a if TYPE_CHECKING if types were declared in Django.

How? Then users would need to also declare the contents of Meta twice, no?

if TYPE_CHECKING:
    class Meta(BaseModelMeta):
        verbose_name = 'bla'
        unique_together = ['example']
else:
    class Meta:
        verbose_name = 'bla'
        unique_together = ['example']

Also, a different thought as I'm writing. MyModel.Meta is never mutated like this suggests, right? Isn't it ported to the MyModel._meta attribute, which is an Option instance?

Sorry, I don't understand. Where is mutation suggested? Yes, the values end up being accessible via the _meta: Option attribute.

@flaeppe
Copy link
Member

flaeppe commented Feb 23, 2023

Sorry, it was a bit unclear regarding the mutation stuff. I think these are the relevant lines:

https://github.com/django/django/blob/51c9bb7cd16081133af4f0ab6d06572660309730/django/db/models/base.py#L110-L124

It kind of seems that class Meta is following some sort of protocol, declared by Options. Since Options will raise on passing invalid attributes:

https://github.com/django/django/blob/51c9bb7cd16081133af4f0ab6d06572660309730/django/db/models/options.py#L230-L234

So the only gain we can have here, currently, is correct types on those Options attributes. Since contribute_to_class for Options will raise during django.setup(), which our plugin calls. Just trying to say that the protocol for Meta is already handled runtime, and will make our plugin crash before it could mention any errors about it.

@flaeppe
Copy link
Member

flaeppe commented Feb 23, 2023

How? Then users would need to also declare the contents of Meta twice, no?

if TYPE_CHECKING:
    class Meta(BaseModelMeta):
        verbose_name = 'bla'
        unique_together = ['example']
else:
    class Meta:
        verbose_name = 'bla'
        unique_together = ['example']

Haha, yeah, that's a bit off. But what I mean is related to the protocol for Meta, I'm not sure how or if there exists enough support to describe it with the current typing system. But it would be described by Django's models.Model, I'd imagine.

Point is, that it's some protocol with optional attributes which should be attached as an attribute on models.Model. And I'm not aware of any support for something like 'partial protocols', i.e. not requiring all attributes.

Something like

class MetaProtocol(Protocol):
    abstract: bool
    ordering: Sequence[str]
    ...

class Model(...):
    ...
    Meta: type[MetaProtocol]

Above is not correct, I'm just trying to display what I think is a limitation within the type system.

@intgr
Copy link
Collaborator Author

intgr commented Feb 23, 2023

Just trying to say that the protocol for Meta is already handled runtime, and will make our plugin crash before it could mention any errors about it.

Ah not really. AFAICT it just checks that there aren't any unrecognized attribute names. It doesn't check the types of attributes values. Some Meta types are checked in system checks, but not all of them.

class Model(...):
    ...
    Meta: type[MetaProtocol]

Ahh yes, supporting that would be very cool. If it's not supported, I wonder if there's already a feature request in mypy.

@flaeppe
Copy link
Member

flaeppe commented Feb 23, 2023

In the end, perhaps the approach here is as close as we'll get. Since Django will error during runtime on invalid attributes, providing a parent class with the types from Options, which your declared Meta will be converted to.

And in practice, what happens, is like if you'd inherit a Meta with defaults from models.Model.

@flaeppe
Copy link
Member

flaeppe commented Feb 23, 2023

Ah not really. AFAICT it just checks that there aren't any unrecognized attribute names. It doesn't check the types of attributes values. Some Meta types are checked in system checks, but not all of them.

That's what I meant, but having incorrect types might make Django crash on some of the attributes. Which will happen before the type checking can tell there's an incorrect type. But that solely depends on what Django does with the attribute in question.

@flaeppe
Copy link
Member

flaeppe commented Feb 23, 2023

Ahh yes, supporting that would be very cool. If it's not supported, I wonder if there's already a feature request in mypy.

I found some info on "partial protocols"/optional members in PEP 544 # Support optional protocol members. But it's suggested as a "future revisit"

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

I like the idea!

I think that we can later explore other options (including plugins, Meta: SomeProtocol definitions in Model type, etc).

But, I think that this is a farly simple solution that clearly solves a given problem!

django_stubs_ext/django_stubs_ext/db/models.py Outdated Show resolved Hide resolved
@sobolevn
Copy link
Member

And we clearly needs some docs for it.

@intgr
Copy link
Collaborator Author

intgr commented Feb 23, 2023

class Model(...):
    ...
    Meta: type[MetaProtocol]

I looked into this, there are two pieces missing in mypy right now. I opened the latter feature request.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

👍


from django_stubs_ext import StrOrPromise

class BaseModelMeta:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any thoughts on this naming? BaseModelMeta? ModelMetaBase? TypedModelMeta?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also the module name. I chose django_stubs_ext.db.models to match Django's django.db.models.

Copy link
Member

Choose a reason for hiding this comment

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

I like the way it is: db.models.BaseModelMeta 👍

@intgr intgr merged commit 217c581 into typeddjango:master Apr 8, 2023
@intgr intgr deleted the add-BaseModelMeta-class branch April 8, 2023 16:07
@intgr
Copy link
Collaborator Author

intgr commented Apr 27, 2023

Note: this feature was renamed from BaseModelMeta to TypedModelMeta in #1456

@intgr intgr added the django-stubs-ext Issues or changes involving django-stubs-ext package label Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
django-stubs-ext Issues or changes involving django-stubs-ext package
Development

Successfully merging this pull request may close these issues.

3 participants