From 058a9b371a73e954efbb32cdb4716d7ee2e497b5 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 22 Mar 2023 06:09:56 +0530 Subject: [PATCH 1/3] [`flake8-django`]: Implement rule DJ012 Checks for the order of Model's inner classes, methods and fields as per the Django Style Guide. Django Style Guide: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#model-style --- .../test/fixtures/flake8_django/DJ012.py | 113 ++++++++++++ crates/ruff/src/checkers/ast/mod.rs | 7 + crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/flake8_django/mod.rs | 1 + .../src/rules/flake8_django/rules/helpers.rs | 12 ++ .../ruff/src/rules/flake8_django/rules/mod.rs | 4 + .../rules/unordered_body_content_in_model.rs | 164 ++++++++++++++++++ ..._flake8_django__tests__DJ012_DJ012.py.snap | 57 ++++++ 9 files changed, 360 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_django/DJ012.py create mode 100644 crates/ruff/src/rules/flake8_django/rules/unordered_body_content_in_model.rs create mode 100644 crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ012_DJ012.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_django/DJ012.py b/crates/ruff/resources/test/fixtures/flake8_django/DJ012.py new file mode 100644 index 0000000000000..fc0ffd0cbbeaf --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_django/DJ012.py @@ -0,0 +1,113 @@ +from django.db import models +from django.db.models import Model + + +class StrBeforeRandomField(models.Model): + """Model with `__str__` before a random property.""" + + class Meta: + verbose_name = "test" + verbose_name_plural = "tests" + + def __str__(self): + return "" + + random_property = "foo" + + +class StrBeforeFieldModel(models.Model): + """Model with `__str__` before fields.""" + + class Meta: + verbose_name = "test" + verbose_name_plural = "tests" + + def __str__(self): + return "foobar" + + first_name = models.CharField(max_length=32) + + +class ManagerBeforeField(models.Model): + """Model with manager before fields.""" + + objects = "manager" + + class Meta: + verbose_name = "test" + verbose_name_plural = "tests" + + def __str__(self): + return "foobar" + + first_name = models.CharField(max_length=32) + + +class CustomMethodBeforeStr(models.Model): + """Model with a custom method before `__str__`.""" + + class Meta: + verbose_name = "test" + verbose_name_plural = "tests" + + def my_method(self): + pass + + def __str__(self): + return "foobar" + + +class GetAbsoluteUrlBeforeSave(Model): + """Model with `get_absolute_url` method before `save` method. + + Subclass this directly using the `Model` class. + """ + + def get_absolute_url(self): + pass + + def save(self): + pass + + +class ConstantsAreNotFields(models.Model): + """Model with an assignment to a constant after `__str__`.""" + + first_name = models.CharField(max_length=32) + + class Meta: + verbose_name = "test" + verbose_name_plural = "tests" + + def __str__(self): + pass + + MY_CONSTANT = id(1) + + +class PerfectlyFine(models.Model): + """Model which has everything in perfect order.""" + + first_name = models.CharField(max_length=32) + last_name = models.CharField(max_length=32) + objects = "manager" + + class Meta: + verbose_name = "test" + verbose_name_plural = "tests" + + def __str__(self): + return "Perfectly fine!" + + def save(self, **kwargs): + super(PerfectlyFine, self).save(**kwargs) + + def get_absolute_url(self): + return "http://%s" % self + + def my_method(self): + pass + + @property + def random_property(self): + return "%s" % self diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 56f616d9fa689..0e5ef123ef9f6 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -708,6 +708,13 @@ where self.diagnostics.push(diagnostic); } } + if self + .settings + .rules + .enabled(Rule::DjangoUnorderedBodyContentInModel) + { + flake8_django::rules::unordered_body_content_in_model(self, bases, body); + } if self.settings.rules.enabled(Rule::GlobalStatement) { pylint::rules::global_statement(self, name); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 73f3bceeba6d3..04fb117731615 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -677,6 +677,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Flake8Django, "006") => Rule::DjangoExcludeWithModelForm, (Flake8Django, "007") => Rule::DjangoAllWithModelForm, (Flake8Django, "008") => Rule::DjangoModelWithoutDunderStr, + (Flake8Django, "012") => Rule::DjangoUnorderedBodyContentInModel, (Flake8Django, "013") => Rule::DjangoNonLeadingReceiverDecorator, _ => return None, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 8355d645f964f..fd56ef33ab891 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -620,6 +620,7 @@ ruff_macros::register_rules!( rules::flake8_django::rules::DjangoExcludeWithModelForm, rules::flake8_django::rules::DjangoAllWithModelForm, rules::flake8_django::rules::DjangoModelWithoutDunderStr, + rules::flake8_django::rules::DjangoUnorderedBodyContentInModel, rules::flake8_django::rules::DjangoNonLeadingReceiverDecorator, ); diff --git a/crates/ruff/src/rules/flake8_django/mod.rs b/crates/ruff/src/rules/flake8_django/mod.rs index 2fcc260c0c863..6b530d5e6799f 100644 --- a/crates/ruff/src/rules/flake8_django/mod.rs +++ b/crates/ruff/src/rules/flake8_django/mod.rs @@ -18,6 +18,7 @@ mod tests { #[test_case(Rule::DjangoExcludeWithModelForm, Path::new("DJ006.py"); "DJ006")] #[test_case(Rule::DjangoAllWithModelForm, Path::new("DJ007.py"); "DJ007")] #[test_case(Rule::DjangoModelWithoutDunderStr, Path::new("DJ008.py"); "DJ008")] + #[test_case(Rule::DjangoUnorderedBodyContentInModel, Path::new("DJ012.py"); "DJ012")] #[test_case(Rule::DjangoNonLeadingReceiverDecorator, Path::new("DJ013.py"); "DJ013")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); diff --git a/crates/ruff/src/rules/flake8_django/rules/helpers.rs b/crates/ruff/src/rules/flake8_django/rules/helpers.rs index e6c0842c9d02d..4f952f135852c 100644 --- a/crates/ruff/src/rules/flake8_django/rules/helpers.rs +++ b/crates/ruff/src/rules/flake8_django/rules/helpers.rs @@ -23,6 +23,18 @@ pub fn is_model_form(checker: &Checker, base: &Expr) -> bool { }) } +/// Return `true` if the expression is constructor for a Django model field. +pub fn is_model_field(checker: &Checker, expr: &Expr) -> bool { + checker + .ctx + .resolve_call_path(expr) + .map_or(false, |call_path| { + call_path + .as_slice() + .starts_with(&["django", "db", "models"]) + }) +} + /// Return the name of the field type, if the expression is constructor for a Django model field. pub fn get_model_field_name<'a>(checker: &'a Checker, expr: &'a Expr) -> Option<&'a str> { checker.ctx.resolve_call_path(expr).and_then(|call_path| { diff --git a/crates/ruff/src/rules/flake8_django/rules/mod.rs b/crates/ruff/src/rules/flake8_django/rules/mod.rs index 8f5190a71e417..504f9c14afb7e 100644 --- a/crates/ruff/src/rules/flake8_django/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_django/rules/mod.rs @@ -8,6 +8,9 @@ pub use non_leading_receiver_decorator::{ pub use nullable_model_string_field::{ nullable_model_string_field, DjangoNullableModelStringField, }; +pub use unordered_body_content_in_model::{ + unordered_body_content_in_model, DjangoUnorderedBodyContentInModel, +}; mod all_with_model_form; mod exclude_with_model_form; @@ -16,3 +19,4 @@ mod locals_in_render_function; mod model_without_dunder_str; mod non_leading_receiver_decorator; mod nullable_model_string_field; +mod unordered_body_content_in_model; diff --git a/crates/ruff/src/rules/flake8_django/rules/unordered_body_content_in_model.rs b/crates/ruff/src/rules/flake8_django/rules/unordered_body_content_in_model.rs new file mode 100644 index 0000000000000..51ff71a31fca6 --- /dev/null +++ b/crates/ruff/src/rules/flake8_django/rules/unordered_body_content_in_model.rs @@ -0,0 +1,164 @@ +use std::fmt; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::types::Range; +use rustpython_parser::ast::{Expr, ExprKind, Stmt, StmtKind}; + +use crate::checkers::ast::Checker; + +use super::helpers; + +/// ## What it does +/// Checks for the order of Model's inner classes, methods, and fields as per +/// the [Django Style Guide]. +/// +/// ## Why is it bad? +/// The [Django Style Guide] specifies that the order of Model inner classes, +/// attributes and methods should be as follows: +/// +/// 1. All database fields +/// 2. Custom manager attributes +/// 3. `class Meta` +/// 4. `def __str__()` +/// 5. `def save()` +/// 6. `def get_absolute_url()` +/// 7. Any custom methods +/// +/// ## Examples +/// ```python +/// from django.db import models +/// +/// +/// class StrBeforeFieldModel(models.Model): +/// class Meta: +/// verbose_name = "test" +/// verbose_name_plural = "tests" +/// +/// def __str__(self): +/// return "foobar" +/// +/// first_name = models.CharField(max_length=32) +/// last_name = models.CharField(max_length=40) +/// ``` +/// +/// Use instead: +/// ```python +/// from django.db import models +/// +/// +/// class StrBeforeFieldModel(models.Model): +/// first_name = models.CharField(max_length=32) +/// last_name = models.CharField(max_length=40) +/// +/// class Meta: +/// verbose_name = "test" +/// verbose_name_plural = "tests" +/// +/// def __str__(self): +/// return "foobar" +/// ``` +/// +/// [Django Style Guide]: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#model-style +#[violation] +pub struct DjangoUnorderedBodyContentInModel { + pub elem_type: ContentType, + pub before: ContentType, +} + +impl Violation for DjangoUnorderedBodyContentInModel { + #[derive_message_formats] + fn message(&self) -> String { + let DjangoUnorderedBodyContentInModel { elem_type, before } = self; + format!("Order of model's inner classes, methods, and fields does not follow the Django Style Guide: {elem_type} should come before {before}") + } +} + +#[derive(Debug, Clone, Copy, PartialOrd, Ord, PartialEq, Eq)] +pub enum ContentType { + FieldDeclaration, + ManagerDeclaration, + MetaClass, + StrMethod, + SaveMethod, + GetAbsoluteUrlMethod, + CustomMethod, +} + +impl fmt::Display for ContentType { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ContentType::FieldDeclaration => f.write_str("field declaration"), + ContentType::ManagerDeclaration => f.write_str("manager declaration"), + ContentType::MetaClass => f.write_str("`Meta` class"), + ContentType::StrMethod => f.write_str("`__str__` method"), + ContentType::SaveMethod => f.write_str("`save` method"), + ContentType::GetAbsoluteUrlMethod => f.write_str("`get_absolute_url` method"), + ContentType::CustomMethod => f.write_str("custom method"), + } + } +} + +fn get_element_type(checker: &Checker, element: &StmtKind) -> Option { + match element { + StmtKind::Assign { targets, value, .. } => { + if let ExprKind::Call { func, .. } = &value.node { + if helpers::is_model_field(checker, func) { + return Some(ContentType::FieldDeclaration); + } + } + let Some(expr) = targets.first() else { + return None; + }; + let ExprKind::Name { id, .. } = &expr.node else { + return None; + }; + if id == "objects" { + Some(ContentType::ManagerDeclaration) + } else { + None + } + } + StmtKind::ClassDef { name, .. } => { + if name == "Meta" { + Some(ContentType::MetaClass) + } else { + None + } + } + StmtKind::FunctionDef { name, .. } => match name.as_str() { + "__str__" => Some(ContentType::StrMethod), + "save" => Some(ContentType::SaveMethod), + "get_absolute_url" => Some(ContentType::GetAbsoluteUrlMethod), + _ => Some(ContentType::CustomMethod), + }, + _ => None, + } +} + +/// DJ012 +pub fn unordered_body_content_in_model(checker: &mut Checker, bases: &[Expr], body: &[Stmt]) { + if !bases.iter().any(|base| helpers::is_model(checker, base)) { + return; + } + let mut elements_type_found = Vec::new(); + for element in body.iter() { + let Some(current_element_type) = get_element_type(checker, &element.node) else { + continue; + }; + let Some(&element_type) = elements_type_found + .iter() + .find(|&&element_type| element_type > current_element_type) else { + elements_type_found.push(current_element_type); + continue; + }; + let diagnostic = Diagnostic::new( + DjangoUnorderedBodyContentInModel { + elem_type: current_element_type, + before: element_type, + }, + Range::from(element), + ); + checker.diagnostics.push(diagnostic); + } +} diff --git a/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ012_DJ012.py.snap b/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ012_DJ012.py.snap new file mode 100644 index 0000000000000..392f1d4bc7422 --- /dev/null +++ b/crates/ruff/src/rules/flake8_django/snapshots/ruff__rules__flake8_django__tests__DJ012_DJ012.py.snap @@ -0,0 +1,57 @@ +--- +source: crates/ruff/src/rules/flake8_django/mod.rs +expression: diagnostics +--- +- kind: + name: DjangoUnorderedBodyContentInModel + body: "Order of model's inner classes, methods, and fields does not follow the Django Style Guide: field declaration should come before `Meta` class" + suggestion: ~ + fixable: false + location: + row: 28 + column: 4 + end_location: + row: 28 + column: 48 + fix: ~ + parent: ~ +- kind: + name: DjangoUnorderedBodyContentInModel + body: "Order of model's inner classes, methods, and fields does not follow the Django Style Guide: field declaration should come before manager declaration" + suggestion: ~ + fixable: false + location: + row: 43 + column: 4 + end_location: + row: 43 + column: 48 + fix: ~ + parent: ~ +- kind: + name: DjangoUnorderedBodyContentInModel + body: "Order of model's inner classes, methods, and fields does not follow the Django Style Guide: `__str__` method should come before custom method" + suggestion: ~ + fixable: false + location: + row: 56 + column: 4 + end_location: + row: 57 + column: 23 + fix: ~ + parent: ~ +- kind: + name: DjangoUnorderedBodyContentInModel + body: "Order of model's inner classes, methods, and fields does not follow the Django Style Guide: `save` method should come before `get_absolute_url` method" + suggestion: ~ + fixable: false + location: + row: 69 + column: 4 + end_location: + row: 70 + column: 12 + fix: ~ + parent: ~ + From 68c27ccabf2dfd1e20ab48f9645f5f01986d188b Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 22 Mar 2023 08:05:38 +0530 Subject: [PATCH 2/3] fix: add rule id to JSON schema --- ruff.schema.json | 1 + 1 file changed, 1 insertion(+) diff --git a/ruff.schema.json b/ruff.schema.json index d5f325a0078aa..bad9a19852e29 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1575,6 +1575,7 @@ "DJ007", "DJ008", "DJ01", + "DJ012", "DJ013", "DTZ", "DTZ0", From 14c395c000a07af0dd7f6a6421da9bc714b3bf3c Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 22 Mar 2023 08:31:37 +0530 Subject: [PATCH 3/3] refactor: helper fn to take Context --- .../rules/all_with_model_form.rs | 2 +- .../rules/exclude_with_model_form.rs | 2 +- .../src/rules/flake8_django/rules/helpers.rs | 46 ++++++++----------- .../rules/model_without_dunder_str.rs | 2 +- .../rules/nullable_model_string_field.rs | 2 +- .../rules/unordered_body_content_in_model.rs | 7 ++- 6 files changed, 27 insertions(+), 34 deletions(-) diff --git a/crates/ruff/src/rules/flake8_django/rules/all_with_model_form.rs b/crates/ruff/src/rules/flake8_django/rules/all_with_model_form.rs index 6d8235ce74161..11d0b87422af8 100644 --- a/crates/ruff/src/rules/flake8_django/rules/all_with_model_form.rs +++ b/crates/ruff/src/rules/flake8_django/rules/all_with_model_form.rs @@ -49,7 +49,7 @@ impl Violation for DjangoAllWithModelForm { /// DJ007 pub fn all_with_model_form(checker: &Checker, bases: &[Expr], body: &[Stmt]) -> Option { - if !bases.iter().any(|base| is_model_form(checker, base)) { + if !bases.iter().any(|base| is_model_form(&checker.ctx, base)) { return None; } for element in body.iter() { diff --git a/crates/ruff/src/rules/flake8_django/rules/exclude_with_model_form.rs b/crates/ruff/src/rules/flake8_django/rules/exclude_with_model_form.rs index fe5c97be42849..731cbcc6983f2 100644 --- a/crates/ruff/src/rules/flake8_django/rules/exclude_with_model_form.rs +++ b/crates/ruff/src/rules/flake8_django/rules/exclude_with_model_form.rs @@ -49,7 +49,7 @@ pub fn exclude_with_model_form( bases: &[Expr], body: &[Stmt], ) -> Option { - if !bases.iter().any(|base| is_model_form(checker, base)) { + if !bases.iter().any(|base| is_model_form(&checker.ctx, base)) { return None; } for element in body.iter() { diff --git a/crates/ruff/src/rules/flake8_django/rules/helpers.rs b/crates/ruff/src/rules/flake8_django/rules/helpers.rs index 4f952f135852c..34069d11b9437 100644 --- a/crates/ruff/src/rules/flake8_django/rules/helpers.rs +++ b/crates/ruff/src/rules/flake8_django/rules/helpers.rs @@ -1,43 +1,33 @@ +use ruff_python_ast::context::Context; use rustpython_parser::ast::Expr; -use crate::checkers::ast::Checker; - /// Return `true` if a Python class appears to be a Django model, based on its base classes. -pub fn is_model(checker: &Checker, base: &Expr) -> bool { - checker - .ctx - .resolve_call_path(base) - .map_or(false, |call_path| { - call_path.as_slice() == ["django", "db", "models", "Model"] - }) +pub fn is_model(context: &Context, base: &Expr) -> bool { + context.resolve_call_path(base).map_or(false, |call_path| { + call_path.as_slice() == ["django", "db", "models", "Model"] + }) } /// Return `true` if a Python class appears to be a Django model form, based on its base classes. -pub fn is_model_form(checker: &Checker, base: &Expr) -> bool { - checker - .ctx - .resolve_call_path(base) - .map_or(false, |call_path| { - call_path.as_slice() == ["django", "forms", "ModelForm"] - || call_path.as_slice() == ["django", "forms", "models", "ModelForm"] - }) +pub fn is_model_form(context: &Context, base: &Expr) -> bool { + context.resolve_call_path(base).map_or(false, |call_path| { + call_path.as_slice() == ["django", "forms", "ModelForm"] + || call_path.as_slice() == ["django", "forms", "models", "ModelForm"] + }) } /// Return `true` if the expression is constructor for a Django model field. -pub fn is_model_field(checker: &Checker, expr: &Expr) -> bool { - checker - .ctx - .resolve_call_path(expr) - .map_or(false, |call_path| { - call_path - .as_slice() - .starts_with(&["django", "db", "models"]) - }) +pub fn is_model_field(context: &Context, expr: &Expr) -> bool { + context.resolve_call_path(expr).map_or(false, |call_path| { + call_path + .as_slice() + .starts_with(&["django", "db", "models"]) + }) } /// Return the name of the field type, if the expression is constructor for a Django model field. -pub fn get_model_field_name<'a>(checker: &'a Checker, expr: &'a Expr) -> Option<&'a str> { - checker.ctx.resolve_call_path(expr).and_then(|call_path| { +pub fn get_model_field_name<'a>(context: &'a Context, expr: &'a Expr) -> Option<&'a str> { + context.resolve_call_path(expr).and_then(|call_path| { let call_path = call_path.as_slice(); if !call_path.starts_with(&["django", "db", "models"]) { return None; diff --git a/crates/ruff/src/rules/flake8_django/rules/model_without_dunder_str.rs b/crates/ruff/src/rules/flake8_django/rules/model_without_dunder_str.rs index 124e0efd9994d..770347a28f0e9 100644 --- a/crates/ruff/src/rules/flake8_django/rules/model_without_dunder_str.rs +++ b/crates/ruff/src/rules/flake8_django/rules/model_without_dunder_str.rs @@ -86,7 +86,7 @@ fn checker_applies(checker: &Checker, bases: &[Expr], body: &[Stmt]) -> bool { if is_model_abstract(body) { continue; } - if helpers::is_model(checker, base) { + if helpers::is_model(&checker.ctx, base) { return true; } } diff --git a/crates/ruff/src/rules/flake8_django/rules/nullable_model_string_field.rs b/crates/ruff/src/rules/flake8_django/rules/nullable_model_string_field.rs index 58f05c24a961e..fe90e0bbdc9d8 100644 --- a/crates/ruff/src/rules/flake8_django/rules/nullable_model_string_field.rs +++ b/crates/ruff/src/rules/flake8_django/rules/nullable_model_string_field.rs @@ -85,7 +85,7 @@ fn is_nullable_field<'a>(checker: &'a Checker, value: &'a Expr) -> Option<&'a st return None; }; - let Some(valid_field_name) = helpers::get_model_field_name(checker, func) else { + let Some(valid_field_name) = helpers::get_model_field_name(&checker.ctx, func) else { return None; }; diff --git a/crates/ruff/src/rules/flake8_django/rules/unordered_body_content_in_model.rs b/crates/ruff/src/rules/flake8_django/rules/unordered_body_content_in_model.rs index 51ff71a31fca6..ebefddf9f17e9 100644 --- a/crates/ruff/src/rules/flake8_django/rules/unordered_body_content_in_model.rs +++ b/crates/ruff/src/rules/flake8_django/rules/unordered_body_content_in_model.rs @@ -103,7 +103,7 @@ fn get_element_type(checker: &Checker, element: &StmtKind) -> Option { if let ExprKind::Call { func, .. } = &value.node { - if helpers::is_model_field(checker, func) { + if helpers::is_model_field(&checker.ctx, func) { return Some(ContentType::FieldDeclaration); } } @@ -138,7 +138,10 @@ fn get_element_type(checker: &Checker, element: &StmtKind) -> Option