Skip to content

[DJ] Add django-model-with-dunder-unicode#12607

Closed
Amar1729 wants to merge 1 commit intoastral-sh:mainfrom
Amar1729:rule/django-dunder-unicode
Closed

[DJ] Add django-model-with-dunder-unicode#12607
Amar1729 wants to merge 1 commit intoastral-sh:mainfrom
Amar1729:rule/django-dunder-unicode

Conversation

@Amar1729
Copy link
Copy Markdown
Contributor

@Amar1729 Amar1729 commented Aug 1, 2024

Summary

Derived from a datadog rule, I've implemented a check for a Django model implementing a __unicode__ method. This was a python2-ism that is not used anymore (as of Python 3.0).

This method is probably quite uncommon by now, but i recently ran into a legacy codebase where __unicode__() was still defined and figured it might be helpful to write a simple rule for it. I believe this method was common in Django projects at the time, but it's entirely possibly to have defined a __unicode__ method outside of using Django; as such, I'm not sure if it makes sense to put this rule somewhere else (e.g. PYUP?) and make it more general, to any kind of class. Either way, it's not directly derived from either of those checkers, and so I'm not sure either if DJ014 is necessarily the best number for it. Thoughts?

Test Plan

  • cargo run -p ruff -- check crates/ruff_linter/resources/test/fixtures/flake8_django/DJ014.py --no-cache --preview --select DJ014
  • cargo test

@Amar1729 Amar1729 force-pushed the rule/django-dunder-unicode branch from 5af2392 to 778b4e6 Compare August 1, 2024 06:01
@Amar1729 Amar1729 force-pushed the rule/django-dunder-unicode branch from 778b4e6 to cc67733 Compare August 1, 2024 06:03
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 1, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh added rule Implementing or modifying a lint rule needs-decision Awaiting a decision from a maintainer labels Aug 9, 2024
@charliermarsh
Copy link
Copy Markdown
Member

charliermarsh commented Aug 9, 2024

This looks reasonable to me, but would be helpful to have a second opinion -- maybe @AlexWaygood or @carljm with Django expertise?

@carljm
Copy link
Copy Markdown
Contributor

carljm commented Aug 9, 2024

I think this rule makes sense, but I don't think there should be anything Django specific about it. The __unicode__ method was part of the Python 2 data model and is not part of the Python 3 data model, so it makes sense to lint against its definition anywhere in a Python 3 only code base, as it isn't doing what the author likely thought it might do.

@AlexWaygood
Copy link
Copy Markdown
Member

AlexWaygood commented Aug 9, 2024

Would this already be caught by PLW3201? If not, should it be?

@MichaReiser
Copy link
Copy Markdown
Member

MichaReiser commented Aug 9, 2024 •
edited
Loading

Would this already be caught by PLW3201?

Nope, this doesn't seem to be the case today playground

Including it in PLW3201 does make sense to me if the method was removed entirely. Although it could be nice to have a more specific error message.

@AlexWaygood
Copy link
Copy Markdown
Member

AlexWaygood commented Aug 9, 2024

Including it in PLW3201 does make sense to me if the method was removed entirely.

The special handling of the method has been removed from Python's data model. I wouldn't say that the method was removed, since you can still define it on any class you like, and you won't get an error. It's just that it now behaves as a normal method rather than one that has any special semantics. But anybody defining it on any class is almost certainly doing so erroneously, because they think that it will have special semantics when it won't.

@Amar1729
Copy link
Copy Markdown
Contributor Author

Amar1729 commented Aug 10, 2024 •
edited
Loading

Hm, is there a straightforward way within the PLW3201 ruff code to output an extra message for a specific case? or would it make more sense to introduce a new code eg PLW3202 deprecated-dunder? (are there even any other deprecated dunder methods?*)

*edit: a quick search of the docs indicates some magic methods in py2 documentation that aren't mentioned in docs for py3. i don't know about some of them but __cmp__ for example is also no longer used.

@MichaReiser
Copy link
Copy Markdown
Member

MichaReiser commented Aug 13, 2024

@carljm do you know if there are any other dunder methods that were deprecated as part of Python 3?

@Amar1729 here's an example of a rule that uses different messages.

ruff/crates/ruff_linter/src/rules/pylint/rules/assert_on_string_literal.rs

Lines 28 to 43 in 18452cf

#[violation]
pub struct AssertOnStringLiteral {
kind: Kind,
}
impl Violation for AssertOnStringLiteral {
#[derive_message_formats]
fn message(&self) -> String {
let AssertOnStringLiteral { kind } = self;
match kind {
Kind::Empty => format!("Asserting on an empty string literal will never pass"),
Kind::NonEmpty => format!("Asserting on a non-empty string literal will always pass"),
Kind::Unknown => format!("Asserting on a string literal may have unintended results"),
}
}
}

@AlexWaygood
Copy link
Copy Markdown
Member

AlexWaygood commented Aug 13, 2024 •
edited
Loading

@carljm do you know if there are any other dunder methods that were deprecated as part of Python 3?

Four that I know off, offhand, are __div__, __rdiv__, __idiv__ and __nonzero__. __metaclass__ also has no special meaning in Python 3, though it was never a special method, just a special attribute.

@carljm
Copy link
Copy Markdown
Contributor

carljm commented Aug 13, 2024

In addition to the ones Alex mentioned, these are also all gone in Python 3: __cmp__, __getslice__, __setslice__, __delslice__, __oct__, __hex__, __members__, __methods__, __nonzero__, __rdiv__, __idiv__. There may be others, that's just what I found in https://docs.python.org/3/whatsnew/3.0.html plus some I already knew of (it doesn't seem like they are all listed in that doc.)

@dscorbett
Copy link
Copy Markdown

dscorbett commented Aug 13, 2024

Other removed dunder methods are __coerce__, __long__, and __rcmp__.

@MichaReiser
Copy link
Copy Markdown
Member

MichaReiser commented Aug 14, 2024

Thanks @Amar1729 for creating this PR and triggering this discussion. Considering that PLW3201 already covers this (in preview mode), I suggest extending the rule to give a custom error message if it detects any Python 2 dunder methods. Would you be interested on PRing this change?

@MichaReiser MichaReiser mentioned this pull request Aug 14, 2024
Closed
@MichaReiser
Copy link
Copy Markdown
Member

MichaReiser commented Aug 14, 2024

I created an issue for the improvement

@Amar1729 Amar1729 deleted the rule/django-dunder-unicode branch August 14, 2024 13:50
@Amar1729
Copy link
Copy Markdown
Contributor Author

Amar1729 commented Aug 14, 2024

no problem! a new PR sounds good, I'll see what I can do.

@lucasctl lucasctl mentioned this pull request Sep 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

needs-decision Awaiting a decision from a maintainer rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Amar1729 @charliermarsh @carljm @AlexWaygood @MichaReiser @dscorbett