Include model base subclasses in plugin base class hook condition

[flaeppe]

I have made things!

We now also trigger the base class hook for subclasses of ModelBase.

Not at all sure how or if we should incorporate this into our test suite.

Related issues

Refs: #1668 (comment)

[sobolevn]

Let's add a test case, maybe?

[flaeppe]

Yeah, I just don't know how I could write a reasonable test here.

Perhaps just reveal_type(CustomizedModel().pk) exists?

[sobolevn]

I would go with a realistic example (solving metaclass conflicts):

class CustomMeta(type): ...

class CustomBase(metaclass=CustomMeta): ...
class ModelCustomMeta(modes.ModelBase, CustomMeta): ...

class MyModel(models.Model, CustomBase, metaclass=ModelCustomMeta): ...

I've seen code like this in the wild. Do you agree?

[flaeppe]

I would go with a realistic example (solving metaclass conflicts):

class CustomMeta(type): ...

class CustomBase(metaclass=CustomMeta): ...
class ModelCustomMeta(modes.ModelBase, CustomMeta): ...

class MyModel(models.Model, CustomBase, metaclass=ModelCustomMeta): ...

I've seen code like this in the wild. Do you agree?

Great, I'll add that case! Probably would've taken me ages to come up with that. Not very familiar with metaclass subclassing..

[flaeppe]

Hm, it's not a breaking case though. Since the metaclass is still django.db.models.base.ModelBase

Then I'm not even sure if you could subclass metaclasses?

[sobolevn]

You sure can :)
https://stackoverflow.com/a/61350480/4842742

[flaeppe]

I can't come up with a reasonable case that doesn't use django.db.models.base.Model and the second we use it we don't need to check for subclassing of ModelBase. As mypy will call our hook for each base.

[sobolevn]

@flaeppe

I can't come up with a reasonable case that doesn't use django.db.models.base.Model

Sorry, I am confused. Why do you think that my case in #1670 (comment) is not realistic? I've used it several times when I mixed two metaclasses.

and the second we use it we don't need to check for subclassing of ModelBase

Why? :)

In my example, I am using models.Model as the base class, but then I use a different metaclass (which is just a conflict resolution helper).

So, the metaclass would be not models.ModelBase, but my custom ModelCustomBase. Which is just the test case we need.

I think that I might be missing something obvious :)

[flaeppe]

I'm not saying it isn't realistic. It's just not a case that covers for subclassing of the metaclass in our hook condition.

We're looking at the metaclass of the bases. Which means in your case the hook condition instantly look at models.Model since that's the first base.

[sobolevn]

Yeap, as I said, I was missing something obvious :)

Here's how mypy executes this hook:

        for base_expr in defn.base_type_exprs:
            base_name = self.get_fullname_for_hook(base_expr)
            if base_name:
                hook = self.plugin.get_base_class_hook(base_name)
                if hook:
                    hook(ClassDefContext(defn, base_expr, self))

Given this example:

-   case: test_custom_model_base_metaclass
    main: |
        from myapp.models import MyModel

        m1 = MyModel(field=1)
        reveal_type(m1.pk)
        reveal_type(m1.field)

        m2 = MyModel(field=None)
        reveal_type(m2.pk)
        reveal_type(m2.field)
    installed_apps:
        - myapp
    files:
        - path: myapp/__init__.py
        - path: myapp/models.py
          content: |
              from django.db import models
              from django.db.models.base import ModelBase

              class CustomMeta(type): ...
              class CustomBase(metaclass=CustomMeta): ...
              class ModelCustomMeta(ModelBase, CustomMeta): ...

              class MyModel(models.Model, CustomBase, metaclass=ModelCustomMeta):
                  field = models.IntegerField()

The fullname would be:

• django.db.models.base.Model Gdef/TypeInfo (django.db.models.base.Model)
• myapp.models.CustomBase Gdef/TypeInfo (myapp.models.CustomBase)

And not MyModel as I originally thought.

Thanks a lot, @flaeppe, for bearing with me!
But, since it works this way, I don't think we need to check the subclass of ModelBase.

[flaeppe]

👍

Yeah, lets close this off for now.