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

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

Open
maddrum opened this issue Oct 23, 2024 · 11 comments
Labels
breaking Breaking API change needs-decision Awaiting a decision from a maintainer rule Implementing or modifying a lint rule

Comments

@maddrum
Copy link

maddrum commented Oct 23, 2024

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
Copy link
Member

MichaReiser commented Oct 23, 2024

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
Copy link
Author

maddrum commented Oct 23, 2024

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 MichaReiser added rule Implementing or modifying a lint rule breaking Breaking API change needs-decision Awaiting a decision from a maintainer labels Oct 24, 2024
@MichaReiser
Copy link
Member

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 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?

@maddrum
Copy link
Author

maddrum commented Oct 24, 2024

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
Copy link
Member

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

@AlexWaygood
Copy link
Member

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
Copy link
Contributor

carljm commented Oct 24, 2024

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
Copy link
Member

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
Copy link
Author

maddrum commented Oct 25, 2024

I have raised this

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

@maddrum
Copy link
Author

maddrum commented Oct 25, 2024

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
Copy link

nessita commented Oct 25, 2024

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

Ticket accepted! (Hello, Django Fellow here 👋 )

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking API change needs-decision Awaiting a decision from a maintainer rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

5 participants