Skip to content

Commit

Permalink
[pep8-naming] Allow Django model loads in `non-lowercase-variable-i…
Browse files Browse the repository at this point in the history
…n-function` (`N806`) (#8917)

## Summary

Allows assignments of the form, e.g., `Attachment =
apps.get_model("zerver", "Attachment")`, for better compatibility with
Django.

Closes #7675.

## Test Plan

`cargo test`
  • Loading branch information
charliermarsh authored Nov 30, 2023
1 parent 774c77a commit c324cb6
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 14 deletions.
14 changes: 13 additions & 1 deletion crates/ruff_linter/resources/test/fixtures/pep8_naming/N806.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import collections
from collections import namedtuple
from typing import TypeAlias, TypeVar, NewType, NamedTuple, TypedDict
from typing import Type, TypeAlias, TypeVar, NewType, NamedTuple, TypedDict

GLOBAL: str = "foo"

Expand Down Expand Up @@ -40,3 +40,15 @@ def loop_assign():
global CURRENT_PORT
for CURRENT_PORT in range(5):
pass


def model_assign() -> None:
Bad = apps.get_model("zerver", "Stream") # N806
Attachment = apps.get_model("zerver", "Attachment") # OK
Recipient = apps.get_model("zerver", model_name="Recipient") # OK
Address: Type = apps.get_model("zerver", "Address") # OK

from django.utils.module_loading import import_string

Bad = import_string("django.core.exceptions.ValidationError") # N806
ValidationError = import_string("django.core.exceptions.ValidationError") # OK
16 changes: 3 additions & 13 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,19 +205,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
ExprContext::Store => {
if checker.enabled(Rule::NonLowercaseVariableInFunction) {
if checker.semantic.current_scope().kind.is_function() {
// Ignore globals.
if !checker
.semantic
.current_scope()
.get(id)
.is_some_and(|binding_id| {
checker.semantic.binding(binding_id).is_global()
})
{
pep8_naming::rules::non_lowercase_variable_in_function(
checker, expr, id,
);
}
pep8_naming::rules::non_lowercase_variable_in_function(
checker, expr, id,
);
}
}
if checker.enabled(Rule::MixedCaseVariableInClassScope) {
Expand Down
63 changes: 63 additions & 0 deletions crates/ruff_linter/src/rules/pep8_naming/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use itertools::Itertools;
use ruff_python_ast::call_path::collect_call_path;
use ruff_python_ast::{self as ast, Arguments, Expr, Stmt};

use ruff_python_semantic::SemanticModel;
Expand Down Expand Up @@ -72,6 +73,7 @@ pub(super) fn is_type_alias_assignment(stmt: &Stmt, semantic: &SemanticModel) ->
}
}

/// Returns `true` if the statement is an assignment to a `TypedDict`.
pub(super) fn is_typed_dict_class(arguments: Option<&Arguments>, semantic: &SemanticModel) -> bool {
arguments.is_some_and(|arguments| {
arguments
Expand All @@ -81,6 +83,67 @@ pub(super) fn is_typed_dict_class(arguments: Option<&Arguments>, semantic: &Sema
})
}

/// Returns `true` if a statement appears to be a dynamic import of a Django model.
///
/// For example, in Django, it's common to use `get_model` to access a model dynamically, as in:
/// ```python
/// def migrate_existing_attachment_data(
/// apps: StateApps, schema_editor: BaseDatabaseSchemaEditor
/// ) -> None:
/// Attachment = apps.get_model("zerver", "Attachment")
/// ```
pub(super) fn is_django_model_import(name: &str, stmt: &Stmt, semantic: &SemanticModel) -> bool {
fn match_model_import(name: &str, expr: &Expr, semantic: &SemanticModel) -> bool {
let Expr::Call(ast::ExprCall {
func, arguments, ..
}) = expr
else {
return false;
};

// Match against, e.g., `apps.get_model("zerver", "Attachment")`.
if let Some(call_path) = collect_call_path(func.as_ref()) {
if matches!(call_path.as_slice(), [.., "get_model"]) {
if let Some(argument) =
arguments.find_argument("model_name", arguments.args.len() - 1)
{
if let Some(string_literal) = argument.as_string_literal_expr() {
return string_literal.value.to_str() == name;
}
}
}
}

// Match against, e.g., `import_string("zerver.models.Attachment")`.
if let Some(call_path) = semantic.resolve_call_path(func.as_ref()) {
if matches!(
call_path.as_slice(),
["django", "utils", "module_loading", "import_string"]
) {
if let Some(argument) = arguments.find_argument("dotted_path", 0) {
if let Some(string_literal) = argument.as_string_literal_expr() {
if let Some((.., model)) = string_literal.value.to_str().rsplit_once('.') {
return model == name;
}
}
}
}
}

false
}

match stmt {
Stmt::AnnAssign(ast::StmtAnnAssign {
value: Some(value), ..
}) => match_model_import(name, value.as_ref(), semantic),
Stmt::Assign(ast::StmtAssign { value, .. }) => {
match_model_import(name, value.as_ref(), semantic)
}
_ => false,
}
}

#[cfg(test)]
mod tests {
use super::{is_acronym, is_camelcase, is_mixed_case};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,15 @@ impl Violation for NonLowercaseVariableInFunction {

/// N806
pub(crate) fn non_lowercase_variable_in_function(checker: &mut Checker, expr: &Expr, name: &str) {
// Ignore globals.
if checker
.semantic()
.lookup_symbol(name)
.is_some_and(|id| checker.semantic().binding(id).is_global())
{
return;
}

if checker
.settings
.pep8_naming
Expand All @@ -72,6 +81,7 @@ pub(crate) fn non_lowercase_variable_in_function(checker: &mut Checker, expr: &E
|| helpers::is_typed_dict_assignment(parent, checker.semantic())
|| helpers::is_type_var_assignment(parent, checker.semantic())
|| helpers::is_type_alias_assignment(parent, checker.semantic())
|| helpers::is_django_model_import(name, parent, checker.semantic())
{
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,22 @@ N806.py:13:5: N806 Variable `CONSTANT` in function should be lowercase
14 | _ = 0
|

N806.py:46:5: N806 Variable `Bad` in function should be lowercase
|
45 | def model_assign() -> None:
46 | Bad = apps.get_model("zerver", "Stream") # N806
| ^^^ N806
47 | Attachment = apps.get_model("zerver", "Attachment") # OK
48 | Recipient = apps.get_model("zerver", model_name="Recipient") # OK
|

N806.py:53:5: N806 Variable `Bad` in function should be lowercase
|
51 | from django.utils.module_loading import import_string
52 |
53 | Bad = import_string("django.core.exceptions.ValidationError") # N806
| ^^^ N806
54 | ValidationError = import_string("django.core.exceptions.ValidationError") # OK
|


0 comments on commit c324cb6

Please sign in to comment.