mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-28 12:54:48 +00:00
Support local and dynamic class- and static-method decorators (#8592)
## Summary This brings ruff's behavior in line with what `pep8-naming` already does and thus closes #8397. I had initially implemented this to look at the last segment of a dotted path only when the entry in the `*-decorators` setting started with a `.`, but in the end I thought it's better to remain consistent w/ `pep8-naming` and doing a match against the last segment of the decorator name in any case. If you prefer to diverge from this in favor of less ambiguity in the configuration let me know and I'll change it so you would need to put e.g. `.expression` in the `classmethod-decorators` list. ## Test Plan Tested against the file in the issue linked below, plus the new testcase added in this PR.
This commit is contained in:
parent
565ddebb15
commit
4ebd0bd31e
8 changed files with 279 additions and 33 deletions
|
@ -63,3 +63,33 @@ class PosOnlyClass:
|
||||||
|
|
||||||
def bad_method_pos_only(this, blah, /, self, something: str):
|
def bad_method_pos_only(this, blah, /, self, something: str):
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
|
||||||
|
class ModelClass:
|
||||||
|
@hybrid_property
|
||||||
|
def bad(cls):
|
||||||
|
pass
|
||||||
|
|
||||||
|
@bad.expression
|
||||||
|
def bad(self):
|
||||||
|
pass
|
||||||
|
|
||||||
|
@bad.wtf
|
||||||
|
def bad(cls):
|
||||||
|
pass
|
||||||
|
|
||||||
|
@hybrid_property
|
||||||
|
def good(self):
|
||||||
|
pass
|
||||||
|
|
||||||
|
@good.expression
|
||||||
|
def good(cls):
|
||||||
|
pass
|
||||||
|
|
||||||
|
@good.wtf
|
||||||
|
def good(self):
|
||||||
|
pass
|
||||||
|
|
||||||
|
@foobar.thisisstatic
|
||||||
|
def badstatic(foo):
|
||||||
|
pass
|
||||||
|
|
|
@ -96,6 +96,26 @@ mod tests {
|
||||||
classmethod_decorators: vec![
|
classmethod_decorators: vec![
|
||||||
"classmethod".to_string(),
|
"classmethod".to_string(),
|
||||||
"pydantic.validator".to_string(),
|
"pydantic.validator".to_string(),
|
||||||
|
"expression".to_string(),
|
||||||
|
],
|
||||||
|
..Default::default()
|
||||||
|
},
|
||||||
|
..settings::LinterSettings::for_rule(Rule::InvalidFirstArgumentNameForMethod)
|
||||||
|
},
|
||||||
|
)?;
|
||||||
|
assert_messages!(diagnostics);
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn staticmethod_decorators() -> Result<()> {
|
||||||
|
let diagnostics = test_path(
|
||||||
|
Path::new("pep8_naming").join("N805.py").as_path(),
|
||||||
|
&settings::LinterSettings {
|
||||||
|
pep8_naming: pep8_naming::settings::Settings {
|
||||||
|
staticmethod_decorators: vec![
|
||||||
|
"staticmethod".to_string(),
|
||||||
|
"thisisstatic".to_string(),
|
||||||
],
|
],
|
||||||
..Default::default()
|
..Default::default()
|
||||||
},
|
},
|
||||||
|
|
|
@ -43,4 +43,37 @@ N805.py:64:29: N805 First argument of a method should be named `self`
|
||||||
65 | pass
|
65 | pass
|
||||||
|
|
|
|
||||||
|
|
||||||
|
N805.py:70:13: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
68 | class ModelClass:
|
||||||
|
69 | @hybrid_property
|
||||||
|
70 | def bad(cls):
|
||||||
|
| ^^^ N805
|
||||||
|
71 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:78:13: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
77 | @bad.wtf
|
||||||
|
78 | def bad(cls):
|
||||||
|
| ^^^ N805
|
||||||
|
79 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:86:14: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
85 | @good.expression
|
||||||
|
86 | def good(cls):
|
||||||
|
| ^^^ N805
|
||||||
|
87 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:94:19: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
93 | @foobar.thisisstatic
|
||||||
|
94 | def badstatic(foo):
|
||||||
|
| ^^^ N805
|
||||||
|
95 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
|
||||||
|
|
|
@ -27,4 +27,29 @@ N805.py:64:29: N805 First argument of a method should be named `self`
|
||||||
65 | pass
|
65 | pass
|
||||||
|
|
|
|
||||||
|
|
||||||
|
N805.py:70:13: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
68 | class ModelClass:
|
||||||
|
69 | @hybrid_property
|
||||||
|
70 | def bad(cls):
|
||||||
|
| ^^^ N805
|
||||||
|
71 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:78:13: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
77 | @bad.wtf
|
||||||
|
78 | def bad(cls):
|
||||||
|
| ^^^ N805
|
||||||
|
79 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:94:19: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
93 | @foobar.thisisstatic
|
||||||
|
94 | def badstatic(foo):
|
||||||
|
| ^^^ N805
|
||||||
|
95 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
|
||||||
|
|
|
@ -0,0 +1,71 @@
|
||||||
|
---
|
||||||
|
source: crates/ruff_linter/src/rules/pep8_naming/mod.rs
|
||||||
|
---
|
||||||
|
N805.py:7:20: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
6 | class Class:
|
||||||
|
7 | def bad_method(this):
|
||||||
|
| ^^^^ N805
|
||||||
|
8 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:12:30: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
10 | if False:
|
||||||
|
11 |
|
||||||
|
12 | def extra_bad_method(this):
|
||||||
|
| ^^^^ N805
|
||||||
|
13 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:31:15: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
30 | @pydantic.validator
|
||||||
|
31 | def lower(cls, my_field: str) -> str:
|
||||||
|
| ^^^ N805
|
||||||
|
32 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:35:15: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
34 | @pydantic.validator("my_field")
|
||||||
|
35 | def lower(cls, my_field: str) -> str:
|
||||||
|
| ^^^ N805
|
||||||
|
36 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:64:29: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
62 | pass
|
||||||
|
63 |
|
||||||
|
64 | def bad_method_pos_only(this, blah, /, self, something: str):
|
||||||
|
| ^^^^ N805
|
||||||
|
65 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:70:13: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
68 | class ModelClass:
|
||||||
|
69 | @hybrid_property
|
||||||
|
70 | def bad(cls):
|
||||||
|
| ^^^ N805
|
||||||
|
71 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:78:13: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
77 | @bad.wtf
|
||||||
|
78 | def bad(cls):
|
||||||
|
| ^^^ N805
|
||||||
|
79 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
N805.py:86:14: N805 First argument of a method should be named `self`
|
||||||
|
|
|
||||||
|
85 | @good.expression
|
||||||
|
86 | def good(cls):
|
||||||
|
| ^^^ N805
|
||||||
|
87 | pass
|
||||||
|
|
|
||||||
|
|
||||||
|
|
|
@ -1,3 +1,4 @@
|
||||||
|
use ruff_python_ast::call_path::collect_call_path;
|
||||||
use ruff_python_ast::call_path::from_qualified_name;
|
use ruff_python_ast::call_path::from_qualified_name;
|
||||||
use ruff_python_ast::helpers::map_callable;
|
use ruff_python_ast::helpers::map_callable;
|
||||||
use ruff_python_ast::Decorator;
|
use ruff_python_ast::Decorator;
|
||||||
|
@ -25,20 +26,10 @@ pub fn classify(
|
||||||
let ScopeKind::Class(class_def) = &scope.kind else {
|
let ScopeKind::Class(class_def) = &scope.kind else {
|
||||||
return FunctionType::Function;
|
return FunctionType::Function;
|
||||||
};
|
};
|
||||||
if decorator_list.iter().any(|decorator| {
|
if decorator_list
|
||||||
// The method is decorated with a static method decorator (like
|
.iter()
|
||||||
// `@staticmethod`).
|
.any(|decorator| is_static_method(decorator, semantic, staticmethod_decorators))
|
||||||
semantic
|
{
|
||||||
.resolve_call_path(map_callable(&decorator.expression))
|
|
||||||
.is_some_and(|call_path| {
|
|
||||||
matches!(
|
|
||||||
call_path.as_slice(),
|
|
||||||
["", "staticmethod"] | ["abc", "abstractstaticmethod"]
|
|
||||||
) || staticmethod_decorators
|
|
||||||
.iter()
|
|
||||||
.any(|decorator| call_path == from_qualified_name(decorator))
|
|
||||||
})
|
|
||||||
}) {
|
|
||||||
FunctionType::StaticMethod
|
FunctionType::StaticMethod
|
||||||
} else if matches!(name, "__new__" | "__init_subclass__" | "__class_getitem__")
|
} else if matches!(name, "__new__" | "__init_subclass__" | "__class_getitem__")
|
||||||
// Special-case class method, like `__new__`.
|
// Special-case class method, like `__new__`.
|
||||||
|
@ -50,19 +41,7 @@ pub fn classify(
|
||||||
matches!(call_path.as_slice(), ["", "type"] | ["abc", "ABCMeta"])
|
matches!(call_path.as_slice(), ["", "type"] | ["abc", "ABCMeta"])
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|| decorator_list.iter().any(|decorator| {
|
|| decorator_list.iter().any(|decorator| is_class_method(decorator, semantic, classmethod_decorators))
|
||||||
// The method is decorated with a class method decorator (like `@classmethod`).
|
|
||||||
semantic
|
|
||||||
.resolve_call_path(map_callable(&decorator.expression))
|
|
||||||
.is_some_and( |call_path| {
|
|
||||||
matches!(
|
|
||||||
call_path.as_slice(),
|
|
||||||
["", "classmethod"] | ["abc", "abstractclassmethod"]
|
|
||||||
) || classmethod_decorators
|
|
||||||
.iter()
|
|
||||||
.any(|decorator| call_path == from_qualified_name(decorator))
|
|
||||||
})
|
|
||||||
})
|
|
||||||
{
|
{
|
||||||
FunctionType::ClassMethod
|
FunctionType::ClassMethod
|
||||||
} else {
|
} else {
|
||||||
|
@ -70,3 +49,83 @@ pub fn classify(
|
||||||
FunctionType::Method
|
FunctionType::Method
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Return `true` if a [`Decorator`] is indicative of a static method.
|
||||||
|
fn is_static_method(
|
||||||
|
decorator: &Decorator,
|
||||||
|
semantic: &SemanticModel,
|
||||||
|
staticmethod_decorators: &[String],
|
||||||
|
) -> bool {
|
||||||
|
let decorator = map_callable(&decorator.expression);
|
||||||
|
|
||||||
|
// The decorator is an import, so should match against a qualified path.
|
||||||
|
if semantic
|
||||||
|
.resolve_call_path(decorator)
|
||||||
|
.is_some_and(|call_path| {
|
||||||
|
matches!(
|
||||||
|
call_path.as_slice(),
|
||||||
|
["", "staticmethod"] | ["abc", "abstractstaticmethod"]
|
||||||
|
) || staticmethod_decorators
|
||||||
|
.iter()
|
||||||
|
.any(|decorator| call_path == from_qualified_name(decorator))
|
||||||
|
})
|
||||||
|
{
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
// We do not have a resolvable call path, most likely from a decorator like
|
||||||
|
// `@someproperty.setter`. Instead, match on the last element.
|
||||||
|
if !staticmethod_decorators.is_empty() {
|
||||||
|
if collect_call_path(decorator).is_some_and(|call_path| {
|
||||||
|
call_path.last().is_some_and(|tail| {
|
||||||
|
staticmethod_decorators
|
||||||
|
.iter()
|
||||||
|
.any(|decorator| tail == decorator)
|
||||||
|
})
|
||||||
|
}) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
false
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Return `true` if a [`Decorator`] is indicative of a class method.
|
||||||
|
fn is_class_method(
|
||||||
|
decorator: &Decorator,
|
||||||
|
semantic: &SemanticModel,
|
||||||
|
classmethod_decorators: &[String],
|
||||||
|
) -> bool {
|
||||||
|
let decorator = map_callable(&decorator.expression);
|
||||||
|
|
||||||
|
// The decorator is an import, so should match against a qualified path.
|
||||||
|
if semantic
|
||||||
|
.resolve_call_path(decorator)
|
||||||
|
.is_some_and(|call_path| {
|
||||||
|
matches!(
|
||||||
|
call_path.as_slice(),
|
||||||
|
["", "classmethod"] | ["abc", "abstractclassmethod"]
|
||||||
|
) || classmethod_decorators
|
||||||
|
.iter()
|
||||||
|
.any(|decorator| call_path == from_qualified_name(decorator))
|
||||||
|
})
|
||||||
|
{
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
// We do not have a resolvable call path, most likely from a decorator like
|
||||||
|
// `@someproperty.setter`. Instead, match on the last element.
|
||||||
|
if !classmethod_decorators.is_empty() {
|
||||||
|
if collect_call_path(decorator).is_some_and(|call_path| {
|
||||||
|
call_path.last().is_some_and(|tail| {
|
||||||
|
classmethod_decorators
|
||||||
|
.iter()
|
||||||
|
.any(|decorator| tail == decorator)
|
||||||
|
})
|
||||||
|
}) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
false
|
||||||
|
}
|
||||||
|
|
|
@ -2242,13 +2242,20 @@ pub struct Pep8NamingOptions {
|
||||||
/// in this list takes a `cls` argument as its first argument.
|
/// in this list takes a `cls` argument as its first argument.
|
||||||
///
|
///
|
||||||
/// Expects to receive a list of fully-qualified names (e.g., `pydantic.validator`,
|
/// Expects to receive a list of fully-qualified names (e.g., `pydantic.validator`,
|
||||||
/// rather than `validator`).
|
/// rather than `validator`) or alternatively a plain name which is then matched against
|
||||||
|
/// the last segment in case the decorator itself consists of a dotted name.
|
||||||
#[option(
|
#[option(
|
||||||
default = r#"[]"#,
|
default = r#"[]"#,
|
||||||
value_type = "list[str]",
|
value_type = "list[str]",
|
||||||
example = r#"
|
example = r#"
|
||||||
# Allow Pydantic's `@validator` decorator to trigger class method treatment.
|
classmethod-decorators = [
|
||||||
classmethod-decorators = ["pydantic.validator"]
|
# Allow Pydantic's `@validator` decorator to trigger class method treatment.
|
||||||
|
"pydantic.validator",
|
||||||
|
# Allow SQLAlchemy's dynamic decorators, like `@field.expression`, to trigger class method treatment.
|
||||||
|
"declared_attr",
|
||||||
|
"expression",
|
||||||
|
"comparator",
|
||||||
|
]
|
||||||
"#
|
"#
|
||||||
)]
|
)]
|
||||||
pub classmethod_decorators: Option<Vec<String>>,
|
pub classmethod_decorators: Option<Vec<String>>,
|
||||||
|
@ -2261,7 +2268,8 @@ pub struct Pep8NamingOptions {
|
||||||
/// in this list has no `self` or `cls` argument.
|
/// in this list has no `self` or `cls` argument.
|
||||||
///
|
///
|
||||||
/// Expects to receive a list of fully-qualified names (e.g., `belay.Device.teardown`,
|
/// Expects to receive a list of fully-qualified names (e.g., `belay.Device.teardown`,
|
||||||
/// rather than `teardown`).
|
/// rather than `teardown`) or alternatively a plain name which is then matched against
|
||||||
|
/// the last segment in case the decorator itself consists of a dotted name.
|
||||||
#[option(
|
#[option(
|
||||||
default = r#"[]"#,
|
default = r#"[]"#,
|
||||||
value_type = "list[str]",
|
value_type = "list[str]",
|
||||||
|
|
4
ruff.schema.json
generated
4
ruff.schema.json
generated
|
@ -2147,7 +2147,7 @@
|
||||||
"type": "object",
|
"type": "object",
|
||||||
"properties": {
|
"properties": {
|
||||||
"classmethod-decorators": {
|
"classmethod-decorators": {
|
||||||
"description": "A list of decorators that, when applied to a method, indicate that the method should be treated as a class method (in addition to the builtin `@classmethod`).\n\nFor example, Ruff will expect that any method decorated by a decorator in this list takes a `cls` argument as its first argument.\n\nExpects to receive a list of fully-qualified names (e.g., `pydantic.validator`, rather than `validator`).",
|
"description": "A list of decorators that, when applied to a method, indicate that the method should be treated as a class method (in addition to the builtin `@classmethod`).\n\nFor example, Ruff will expect that any method decorated by a decorator in this list takes a `cls` argument as its first argument.\n\nExpects to receive a list of fully-qualified names (e.g., `pydantic.validator`, rather than `validator`) or alternatively a plain name which is then matched against the last segment in case the decorator itself consists of a dotted name.",
|
||||||
"type": [
|
"type": [
|
||||||
"array",
|
"array",
|
||||||
"null"
|
"null"
|
||||||
|
@ -2177,7 +2177,7 @@
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
"staticmethod-decorators": {
|
"staticmethod-decorators": {
|
||||||
"description": "A list of decorators that, when applied to a method, indicate that the method should be treated as a static method (in addition to the builtin `@staticmethod`).\n\nFor example, Ruff will expect that any method decorated by a decorator in this list has no `self` or `cls` argument.\n\nExpects to receive a list of fully-qualified names (e.g., `belay.Device.teardown`, rather than `teardown`).",
|
"description": "A list of decorators that, when applied to a method, indicate that the method should be treated as a static method (in addition to the builtin `@staticmethod`).\n\nFor example, Ruff will expect that any method decorated by a decorator in this list has no `self` or `cls` argument.\n\nExpects to receive a list of fully-qualified names (e.g., `belay.Device.teardown`, rather than `teardown`) or alternatively a plain name which is then matched against the last segment in case the decorator itself consists of a dotted name.",
|
||||||
"type": [
|
"type": [
|
||||||
"array",
|
"array",
|
||||||
"null"
|
"null"
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue