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

[flake8-django]: Implement rule DJ012 #3659

Merged
merged 3 commits into from
Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
113 changes: 113 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_django/DJ012.py
Original file line number Diff line number Diff line change
@@ -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
7 changes: 7 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Flake8Django, "006") => Rule::DjangoExcludeWithModelForm,
(Flake8Django, "007") => Rule::DjangoAllWithModelForm,
(Flake8Django, "008") => Rule::DjangoModelWithoutDunderStr,
(Flake8Django, "012") => Rule::DjangoUnorderedBodyContentInModel,
(Flake8Django, "013") => Rule::DjangoNonLeadingReceiverDecorator,

_ => return None,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);

Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/flake8_django/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Violation for DjangoAllWithModelForm {

/// DJ007
pub fn all_with_model_form(checker: &Checker, bases: &[Expr], body: &[Stmt]) -> Option<Diagnostic> {
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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ pub fn exclude_with_model_form(
bases: &[Expr],
body: &[Stmt],
) -> Option<Diagnostic> {
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() {
Expand Down
40 changes: 21 additions & 19 deletions crates/ruff/src/rules/flake8_django/rules/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,31 +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(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;
Expand Down
4 changes: 4 additions & 0 deletions crates/ruff/src/rules/flake8_django/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down