Skip to content

Commit

Permalink
[flake8-django] DJ003, DJ006, DJ007 (#3236)
Browse files Browse the repository at this point in the history
Implements [flake8-django](https://github.com/rocioar/flake8-django) rules:
- DJ03
- DJ06
- DJ07
  • Loading branch information
lkh42t authored Feb 26, 2023
1 parent 3a78b59 commit bc79f54
Show file tree
Hide file tree
Showing 17 changed files with 426 additions and 7 deletions.
21 changes: 21 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_django/DJ003.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
from django.shortcuts import render


def test_view1(request):
return render(request, "index.html", locals())


def test_view2(request):
return render(request, "index.html", context=locals())


def test_view3(request):
return render(request, "index.html")


def test_view4(request):
return render(request, "index.html", {})


def test_view5(request):
return render(request, "index.html", context={})
11 changes: 11 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_django/DJ006.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from django import forms


class TestModelForm1(forms.ModelForm):
class Meta:
exclude = ["bar"]


class TestModelForm2(forms.ModelForm):
class Meta:
fields = ["foo"]
16 changes: 16 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_django/DJ007.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from django import forms


class TestModelForm1(forms.ModelForm):
class Meta:
fields = "__all__"


class TestModelForm2(forms.ModelForm):
class Meta:
fields = b"__all__"


class TestModelForm3(forms.ModelForm):
class Meta:
fields = ["foo"]
20 changes: 20 additions & 0 deletions crates/ruff/src/checkers/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -812,6 +812,21 @@ where
self, body,
));
}

if self.settings.rules.enabled(&Rule::ExcludeWithModelForm) {
if let Some(diagnostic) =
flake8_django::rules::exclude_with_model_form(self, bases, body)
{
self.diagnostics.push(diagnostic);
}
}
if self.settings.rules.enabled(&Rule::AllWithModelForm) {
if let Some(diagnostic) =
flake8_django::rules::all_with_model_form(self, bases, body)
{
self.diagnostics.push(diagnostic);
}
}
if self.settings.rules.enabled(&Rule::ModelWithoutDunderStr) {
if let Some(diagnostic) =
flake8_django::rules::model_without_dunder_str(self, bases, body, stmt)
Expand Down Expand Up @@ -2941,6 +2956,11 @@ where
{
pylint::rules::logging_call(self, func, args, keywords);
}

// flake8-django
if self.settings.rules.enabled(&Rule::LocalsInRenderFunction) {
flake8_django::rules::locals_in_render_function(self, func, args, keywords);
}
}
ExprKind::Dict { keys, values } => {
if self
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,9 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {

// flake8-django
(Flake8Django, "001") => Rule::NullableModelStringField,
(Flake8Django, "003") => Rule::LocalsInRenderFunction,
(Flake8Django, "006") => Rule::ExcludeWithModelForm,
(Flake8Django, "007") => Rule::AllWithModelForm,
(Flake8Django, "008") => Rule::ModelWithoutDunderStr,
(Flake8Django, "013") => Rule::NonLeadingReceiverDecorator,

Expand Down
3 changes: 3 additions & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,9 @@ ruff_macros::register_rules!(
rules::ruff::rules::UnusedNOQA,
// flake8-django
rules::flake8_django::rules::NullableModelStringField,
rules::flake8_django::rules::LocalsInRenderFunction,
rules::flake8_django::rules::ExcludeWithModelForm,
rules::flake8_django::rules::AllWithModelForm,
rules::flake8_django::rules::ModelWithoutDunderStr,
rules::flake8_django::rules::NonLeadingReceiverDecorator,
);
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff/src/rules/flake8_django/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ mod tests {
use crate::test::test_path;

#[test_case(Rule::NullableModelStringField, Path::new("DJ001.py"); "DJ001")]
#[test_case(Rule::LocalsInRenderFunction, Path::new("DJ003.py"); "DJ003")]
#[test_case(Rule::ExcludeWithModelForm, Path::new("DJ006.py"); "DJ006")]
#[test_case(Rule::AllWithModelForm, Path::new("DJ007.py"); "DJ007")]
#[test_case(Rule::ModelWithoutDunderStr, Path::new("DJ008.py"); "DJ008")]
#[test_case(Rule::NonLeadingReceiverDecorator, Path::new("DJ013.py"); "DJ013")]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
Expand Down
96 changes: 96 additions & 0 deletions crates/ruff/src/rules/flake8_django/rules/all_with_model_form.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
use rustpython_parser::ast::{Constant, Expr, ExprKind, Stmt, StmtKind};

use ruff_macros::{define_violation, derive_message_formats};

use crate::rules::flake8_django::rules::helpers::is_model_form;
use crate::violation::Violation;
use crate::{checkers::ast::Checker, registry::Diagnostic, Range};

define_violation!(
/// ## What it does
/// Checks for the use of `fields = "__all__"` in Django `ModelForm`
/// classes.
///
/// ## Why is this bad?
/// If a `ModelForm` includes the `fields = "__all__"` attribute, any new
/// field that is added to the model will automatically be exposed for
/// modification.
///
/// ## Example
/// ```python
/// from django.forms import ModelForm
///
/// class PostForm(ModelForm):
/// class Meta:
/// model = Post
/// fields = "__all__"
/// ```
///
/// Use instead:
/// ```python
/// from django.forms import ModelForm
///
/// class PostForm(ModelForm):
/// class Meta:
/// model = Post
/// fields = ["title", "content"]
/// ```
pub struct AllWithModelForm;
);
impl Violation for AllWithModelForm {
#[derive_message_formats]
fn message(&self) -> String {
format!("Do not use `__all__` with `ModelForm`, use `fields` instead")
}
}

/// 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)) {
return None;
}
for element in body.iter() {
let StmtKind::ClassDef { name, body, .. } = &element.node else {
continue;
};
if name != "Meta" {
continue;
}
for element in body.iter() {
let StmtKind::Assign { targets, value, .. } = &element.node else {
continue;
};
for target in targets.iter() {
let ExprKind::Name { id, .. } = &target.node else {
continue;
};
if id != "fields" {
continue;
}
let ExprKind::Constant { value, .. } = &value.node else {
continue;
};
match &value {
Constant::Str(s) => {
if s == "__all__" {
return Some(Diagnostic::new(
AllWithModelForm,
Range::from_located(element),
));
}
}
Constant::Bytes(b) => {
if b == "__all__".as_bytes() {
return Some(Diagnostic::new(
AllWithModelForm,
Range::from_located(element),
));
}
}
_ => (),
};
}
}
}
None
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
use rustpython_parser::ast::{Expr, ExprKind, Stmt, StmtKind};

use ruff_macros::{define_violation, derive_message_formats};

use crate::rules::flake8_django::rules::helpers::is_model_form;
use crate::violation::Violation;
use crate::{checkers::ast::Checker, registry::Diagnostic, Range};

define_violation!(
/// ## What it does
/// Checks for the use of `exclude` in Django `ModelForm` classes.
///
/// ## Why is this bad?
/// If a `ModelForm` includes the `exclude` attribute, any new field that
/// is added to the model will automatically be exposed for modification.
///
/// ## Example
/// ```python
/// from django.forms import ModelForm
///
/// class PostForm(ModelForm):
/// class Meta:
/// model = Post
/// exclude = ["author"]
/// ```
///
/// Use instead:
/// ```python
/// from django.forms import ModelForm
///
/// class PostForm(ModelForm):
/// class Meta:
/// model = Post
/// fields = ["title", "content"]
/// ```
pub struct ExcludeWithModelForm;
);
impl Violation for ExcludeWithModelForm {
#[derive_message_formats]
fn message(&self) -> String {
format!("Do not use `exclude` with `ModelForm`, use `fields` instead")
}
}

/// DJ006
pub fn exclude_with_model_form(
checker: &Checker,
bases: &[Expr],
body: &[Stmt],
) -> Option<Diagnostic> {
if !bases.iter().any(|base| is_model_form(checker, base)) {
return None;
}
for element in body.iter() {
let StmtKind::ClassDef { name, body, .. } = &element.node else {
continue;
};
if name != "Meta" {
continue;
}
for element in body.iter() {
let StmtKind::Assign { targets, .. } = &element.node else {
continue;
};
for target in targets.iter() {
let ExprKind::Name { id, .. } = &target.node else {
continue;
};
if id == "exclude" {
return Some(Diagnostic::new(
ExcludeWithModelForm,
Range::from_located(target),
));
}
}
}
}
None
}
11 changes: 10 additions & 1 deletion crates/ruff/src/rules/flake8_django/rules/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,22 @@ use rustpython_parser::ast::Expr;

use crate::checkers::ast::Checker;

/// Return `true` if a Python class appears to be a Django model based on a base class.
/// 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.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.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 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.resolve_call_path(expr).and_then(|call_path| {
let call_path = call_path.as_slice();
Expand Down
Loading

0 comments on commit bc79f54

Please sign in to comment.