astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.24k stars 1.08k forks source link

DJ012 produces false positive when there are other dunder methods in model class #13892

Open maddrum opened 4 days ago

maddrum commented 4 days ago

ruff check produces false positive DJ012 for Django models like so:

class Person(models.Model):
    name = models.CharField(xxxx)    

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        <some custom code>

    def save(*args, **kwargs):
        super().save(*args, `**kwargs)

According to Python best practices, dunder methods should always be kept at the beginning of the class. But since save() is a Django method which is after other methods method it invokes error.

DJ012 should obey other Python class structuring, before evaluating Django styleguide.

MichaReiser commented 4 days ago

I'm not a Django expert, so it's unclear to me why the Django style guide only mentions the ordering of __str__. Unless all other dunder methods are included in the Any custom methods section.

It seems overriding __init__ isn't recommended.

maddrum commented 4 days ago

I'm not a Django expert, so it's unclear to me why the Django style guide only mentions the ordering of __str__. Unless all other dunder methods are included in the Any custom methods section.

It seems overriding __init__ isn't recommended.

__init__ is only for display purposes, that is going to be true for all dunder methods.

MichaReiser commented 3 days ago

I want to make sure that we're 100% sure that this change is in conformance with the style guide because it is a breaking change: All dunder methods other than __str__ would suddenly be flagged as incorrectly ordered.

As I said before, I'm not a Django expert but I did some extra research.

The impression I get is that Ruff's current behavior is correct. My suspicious is that overriding other dunder methods is just very uncommon (or not advised) for Django models. Could you share some examples (from your project and/or other projects) where it is necessary to override additional dunder methods?

maddrum commented 3 days ago

I want to make sure that we're 100% sure that this change is in conformance with the style guide because it is a breaking change: All dunder methods other than __str__ would suddenly be flagged as incorrectly ordered.

As I said before, I'm not a Django expert but I did some extra research.

* The [upstream rule](https://github.com/rocioar/flake8-django/blob/895364969af49cbdd3c6ba8e63adeb9e4415ec5d/flake8_django/checkers/model_content_order.py#L77C5-L85) implements the same ordering as Ruff

* The django style guide and model documentation only mention overriding `__str__` and warn about overriding `__init__`

The impression I get is that Ruff's current behavior is correct. My suspicious is that overriding other dunder methods is just very uncommon (or not advised) for Django models. Could you share some examples (from your project and/or other projects) where it is necessary to override additional dunder methods?

For example, you might want to define __repr__ in your Django model, which is safe and okay and has common usage in logging.

For __init__ there is a semi-dirty solution/which is a lot common/ for tracking model field updates, since Django does not have a solution for that on the model level/it has on forms, but that might not be a solution in every case/

More on that - https://medium.com/@mmzeynalli/how-to-detect-field-changes-in-django-ae4bc719aea2 Solution 3.

To summarize, in real-world scenarios, there are a lot of cases where that style guide is impossible to follow, which is why this check is going to produce fake positives

MichaReiser commented 3 days ago

@AlexWaygood any chance that you're familiar with DJango?

AlexWaygood commented 3 days ago

I think @carljm is almost certainly our best django expert on the team 😄 Whereas I have never used django, Carl was both a member of the django core team (maybe still is?) and helped maintain a very large django-based codebase for many years (Instagram)

carljm commented 3 days ago

Possibly we should open an issue on Django itself to get the style guide clarified? I assume the upstream rule was simply based on a literal reading of the style guide as currently written. Although overriding __init__ specifically may be discouraged on Django models, there are certainly other dunder methods that may make sense to override, so the style guide should offer guidance on where they belong.

Personally I agree with @maddrum that the typical Python convention to put dunder methods first should be respected here, and __str__ should be replaced with "__str__ or any other Python dunder methods" in the Django style guide. But I don't think this is really our call to make. The rule is intended to encode the style guide, and the style guide is simply not clear here.

MichaReiser commented 2 days ago

Thanks @carljm. This is a great point. @maddrum would you be interested in opening an issue on the django style guide project where you ask them to clarify the ordering?

maddrum commented 2 days ago

I have raised this

https://code.djangoproject.com/ticket/35866#ticket

maddrum commented 2 days ago

Possibly we should open an issue on Django itself to get the style guide clarified? I assume the upstream rule was simply based on a literal reading of the style guide as currently written. Although overriding __init__ specifically may be discouraged on Django models, there are certainly other dunder methods that may make sense to override, so the style guide should offer guidance on where they belong.

Personally I agree with @maddrum that the typical Python convention to put dunder methods first should be respected here, and __str__ should be replaced with "__str__ or any other Python dunder methods" in the Django style guide. But I don't think this is really our call to make. The rule is intended to encode the style guide, and the style guide is simply not clear here.

This one applies to staticmethods, private methods, properties and a lot of other methods that come on top. The proposed structure violates many practices and that error currently requires restyling code not according to Python directives. And Django code should follow those first and then apply other rules.

nessita commented 2 days ago

Possibly we should open an issue on Django itself to get the style guide clarified?

Ticket accepted! (Hello, Django Fellow here :wave: )

Personally I agree with @maddrum that the typical Python convention to put dunder methods first should be respected here, and __str__ should be replaced with "__str__ or any other Python dunder methods" in the Django style guide. But I don't think this is really our call to make. The rule is intended to encode the style guide, and the style guide is simply not clear here.

As I put in the ticket, my reading of the Django docs for "any custom method" is "any custom business logic or helper method that you create for your model", and it does not apply to overrides of what object provides or any other documented dunder method.

I accepted the ticket on the basis that the docs could be more clear about this. Thanks everyone!