Add ClassVar annotations on mutable ModelAdmin types for ruff

[zyv]

I have made things!

I suggest adding ClassVar annotations on mutable ModelAdmin types to silence the ruff rule RUF012:

https://docs.astral.sh/ruff/rules/mutable-class-default/

This rule requires that mutable types are explicitly marked as class (as opposed to instance) variables. There is some discussion in the referenced issue (astral-sh/ruff#5243) as to whether or not this rule should apply to DSL-type classes at all.

When writing Django code, I think it's generally easy to solve the problem by switching from lists to tuples for user-written code, but dictionaries are more tricky. Adding ClassVar annotations takes care of the dictionary story as well. It's not even wrong!

As far as migrations are concerned, either Ruff should add exceptions based on class hierarchy, as they apparently did for Pydantic, or maybe the lists can be marked as ClassVar here. I'm not sure how you (and the Ruff developers) feel about this, so I'm not adding any more types at the moment. Besides, the problem can be solved by per-file excludes:

[tool.ruff.lint.per-file-ignores]
"**/migrations/**.py" = ["RUF012"]

Related issues

Refs astral-sh/ruff#5243.

[sobolevn]

Please, fix the CI. What about other fields? It would be strange to have just two ClassVars and others as regular intance fields.

[zyv]

Hey Nikita! Cheers to Nizhny.

Please, fix the CI.

Thanks for the very quick response! I'll look into it.

What about other fields? It would be strange to have just two ClassVars and others as regular intance fields.

You tell me :) That's what I asked in the PR description.

• Do you want only these fields to be marked as ClassVar?
• Do you want all potentially mutable fields (Sequence) to be marked as ClassVar?
• Do you want all fields, mutable or not, to be marked as ClassVar?

[sobolevn]

You tell me :) That's what I asked in the PR description.
Do you want all fields, mutable or not, to be marked as ClassVar?

I want fields that are really ClassVar fields to be ClassVar. But, there's a problem here.
What is a ClassVar field?

class Some:
    x: ClassVar[int] = 1

    @classmethod
    def print_x(cls) -> None:
        print(cls.x)  # `x` cannot be an instance field, because we have explicit `cls.x` usage

But, if some field can be both instance and class level, then this would be an error:

class Some:
    x: ClassVar[int] = 1
    def __init__(self) -> None:
        self.x = 2  # error: instance level overrides a classvar

So, in this case - I think that ruff does not really help, when forcing people to think about mutability, when you should think about usage.

As the result, if some fields are class-level fields, then let's add annotations. If not - let's keep this as-is.

[zyv]

So, in this case - I think that ruff does not really help, when forcing people to think about mutability, when you should think about usage.

I think I understand where ruff is coming from though, having been bitten by mutable default arguments myself a few decades ago at the beginning of my Python "career"...

As the result, if some fields are class-level fields, then let's add annotations. If not - let's keep this as-is.

Thank you for the explanation and examples. It was very helpful. I agree with you that it makes sense to mark fields with ClassVar according to their intended use.

I went through the Django documentation here and marked all the fields listed there as configuration class variables:

https://docs.djangoproject.com/en/dev/ref/contrib/admin/#modeladmin-options

I had to leave out actions, list_display and list_display_links because ClassVar cannot contain type variables. Do you have a solution for this?

Also, this idiom is quite common in Django. Do you have any idea what else you think should be marked? I think it could include the following classes:

• Models.Model
• Meta
• migrations.Migration
• View
• forms.ModelForm / forms.Form
• Media

This would probably be too much work for me alone. And maybe better submitted as separate PRs anyway.

[sobolevn]

I had to leave out actions, list_display and list_display_links because ClassVar cannot contain type variables. Do you have a solution for this?

Nope, that's a limitation of our type system.

[sobolevn]

Please, add a comment: why do we left these two without a ClassVar, so future users won't be confused.

[zyv]

Added in 3e2fdc8.

[flaeppe]

I wonder if any of the attributes have any use case to be modified outside/after class declaration.

Though changing to ClassVar is making it stricter, we could always relax it later on if needed.

I don't have any strong opinions here

[zyv]

I wonder if any of the attributes have any use case to be modified outside/after class declaration.

Yes, I wondered about this as well and actually checked the Django source code. It seems that this is not the case. Of course, this is not necessarily true for user code.

However, the usual pattern seems to be "if you need to override something, don't touch the class variables, override get_...() functions".

Maybe some real user feedback will come after the updated stubs make it into PyCharm. Do you have any idea how often this happens?

Until then, it might not be a good idea to process the remaining classes. I think it's worth looking at Migrations, but then wait with the rest.