Skip to content

Commit

Permalink
Fix {class,static}method-decorators for dotted names
Browse files Browse the repository at this point in the history
  • Loading branch information
ThiefMaster committed Nov 9, 2023
1 parent 565ddeb commit 335c989
Show file tree
Hide file tree
Showing 7 changed files with 219 additions and 10 deletions.
30 changes: 30 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pep8_naming/N805.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,33 @@ def good_method_pos_only(self, blah, /, something: str):

def bad_method_pos_only(this, blah, /, self, something: str):
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
20 changes: 20 additions & 0 deletions crates/ruff_linter/src/rules/pep8_naming/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,26 @@ mod tests {
classmethod_decorators: vec![
"classmethod".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()
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,37 @@ N805.py:64:29: N805 First argument of a method should be named `self`
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
|


Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,29 @@ N805.py:64:29: N805 First argument of a method should be named `self`
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
|


Original file line number Diff line number Diff line change
@@ -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
|


44 changes: 36 additions & 8 deletions crates/ruff_python_semantic/src/analyze/function_type.rs
Original file line number Diff line number Diff line change
@@ -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::helpers::map_callable;
use ruff_python_ast::Decorator;
Expand Down Expand Up @@ -28,16 +29,29 @@ pub fn classify(
if decorator_list.iter().any(|decorator| {
// The method is decorated with a static method decorator (like
// `@staticmethod`).
semantic
.resolve_call_path(map_callable(&decorator.expression))
.is_some_and(|call_path| {
let real_decorator = map_callable(&decorator.expression);
match semantic.resolve_call_path(real_decorator) {
Some(call_path) => {
matches!(
call_path.as_slice(),
["", "staticmethod"] | ["abc", "abstractstaticmethod"]
) || staticmethod_decorators
.iter()
.any(|decorator| call_path == from_qualified_name(decorator))
})
}
// We do not have a resolvable call path, most likely from a decorator similar
// to `@someproperty.setter` - for those we match the last element.
// It would be nicer to use e.g. `.whatever` in the `staticmethod-decorators` config
// option but `pep8-naming` doesn't do this either.
// XXX: It's quite unlikely that this is ever used for staticmethods, but we need it
// for classmethods below and like this the behavior is consistent.
None => collect_call_path(real_decorator).is_some_and(|call_path| {
let tail = call_path.as_slice().last().unwrap();
staticmethod_decorators
.iter()
.any(|decorator| tail == &decorator)
}),
}
}) {
FunctionType::StaticMethod
} else if matches!(name, "__new__" | "__init_subclass__" | "__class_getitem__")
Expand All @@ -52,16 +66,30 @@ pub fn classify(
})
|| decorator_list.iter().any(|decorator| {
// The method is decorated with a class method decorator (like `@classmethod`).
semantic
.resolve_call_path(map_callable(&decorator.expression))
.is_some_and( |call_path| {
let real_decorator = map_callable(&decorator.expression);
match semantic.resolve_call_path(real_decorator) {
Some(call_path) => {
matches!(
call_path.as_slice(),
["", "classmethod"] | ["abc", "abstractclassmethod"]
) || classmethod_decorators
.iter()
.any(|decorator| call_path == from_qualified_name(decorator))
})
},
// We do not have a resolvable call path, most likely from a decorator similar
// to `@someproperty.setter` - for those we match the last element.
// It would be nicer to use e.g. `.whatever` in the `classmethod-decorators` config
// option but `pep8-naming` doesn't do this either.
None => {
collect_call_path(real_decorator)
.is_some_and(|call_path| {
let tail = call_path.as_slice().last().unwrap();
classmethod_decorators
.iter()
.any(|decorator| tail == &decorator)
})
}
}
})
{
FunctionType::ClassMethod
Expand Down
6 changes: 4 additions & 2 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2242,7 +2242,8 @@ pub struct Pep8NamingOptions {
/// in this list takes a `cls` argument as its first argument.
///
/// 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(
default = r#"[]"#,
value_type = "list[str]",
Expand All @@ -2261,7 +2262,8 @@ pub struct Pep8NamingOptions {
/// in this list has no `self` or `cls` argument.
///
/// 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(
default = r#"[]"#,
value_type = "list[str]",
Expand Down

0 comments on commit 335c989

Please sign in to comment.