-
Notifications
You must be signed in to change notification settings - Fork 938
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
Use function_type::classify
for yield-in-init
#2742
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,9 @@ | ||
use ruff_macros::{define_violation, derive_message_formats}; | ||
use rustpython_parser::ast::Expr; | ||
|
||
use ruff_macros::{define_violation, derive_message_formats}; | ||
|
||
use crate::ast::function_type; | ||
use crate::ast::function_type::FunctionType; | ||
use crate::{ | ||
ast::types::{FunctionDef, Range, ScopeKind}, | ||
checkers::ast::Checker, | ||
|
@@ -9,6 +12,19 @@ use crate::{ | |
}; | ||
|
||
define_violation!( | ||
/// ### What it does | ||
/// Checks for `__init__.py` methods that turned into generators | ||
/// via the presence of `yield` or `yield from` statements. | ||
/// | ||
/// ### Why is this bad? | ||
/// Generators are not allowed in `__init__.py` methods. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even without that mistake I find that description to be very bad. Firstly it is misleading because generators can be used in Secondly "is not allowed" is just completely nondescript ... what does that mean? Not allowed by whom? I think GitHub's CodeQL documentation does a better job:
The last sentence here is the important part. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, I'll fix it up! Though, in my opinion, calling something "very bad" is not a productive way to deliver feedback and is a good way to turn a discussion into an argument. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The documentation is written in a voice of authority ... when we use such a voice I think we should take great care that what we are writing is correct, clear and informative because otherwise we are very much bound to cause confusion or waste time of users who take our documentation at face value. I think accurately and clearly answering There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree! Nowhere did I disagree that the documentation here could be improved. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if it's ok for us to copy the documentationfrom GitHub CodeQL in verbatim ... while you did add a reference in e5f5142 it's not clear that the section is from there. I guess this could be clarified with a Markdown footnote ... but I guess it also raises some licensing questions ... is the licensing of our documentation more lax than of our code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought a reference was sufficient but I don't want there to be any ambiguity so I'll just rewrite it entirely. |
||
/// | ||
/// ### Example | ||
/// ```python | ||
/// class Foo: | ||
/// def __init__(self): | ||
/// yield 1 | ||
/// ``` | ||
pub struct YieldInInit; | ||
); | ||
|
||
|
@@ -21,23 +37,38 @@ impl Violation for YieldInInit { | |
|
||
/// PLE0100 | ||
pub fn yield_in_init(checker: &mut Checker, expr: &Expr) { | ||
let parent_scope_is_class: Option<bool> = | ||
checker | ||
.current_scopes() | ||
.nth(1) | ||
.and_then(|scope| match scope.kind { | ||
ScopeKind::Class(..) => Some(true), | ||
_ => None, | ||
}); | ||
|
||
let current_scope_is_init = match checker.current_scope().kind { | ||
ScopeKind::Function(FunctionDef { name, .. }) => Some(name == "__init__"), | ||
_ => None, | ||
let scope = checker.current_scope(); | ||
let ScopeKind::Function(FunctionDef { | ||
name, | ||
decorator_list, | ||
.. | ||
}) = &scope.kind else { | ||
return; | ||
}; | ||
|
||
if *name != "__init__" { | ||
return; | ||
} | ||
|
||
let Some(parent) = checker.current_scope_parent() else { | ||
return; | ||
}; | ||
|
||
if parent_scope_is_class == Some(true) && current_scope_is_init == Some(true) { | ||
checker | ||
.diagnostics | ||
.push(Diagnostic::new(YieldInInit, Range::from_located(expr))); | ||
if !matches!( | ||
function_type::classify( | ||
checker, | ||
parent, | ||
name, | ||
decorator_list, | ||
&checker.settings.pep8_naming.classmethod_decorators, | ||
&checker.settings.pep8_naming.staticmethod_decorators, | ||
), | ||
FunctionType::Method | ||
) { | ||
return; | ||
} | ||
|
||
checker | ||
.diagnostics | ||
.push(Diagnostic::new(YieldInInit, Range::from_located(expr))); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# yield-in-init (PLE0100) | ||
|
||
Derived from the **Pylint** linter. | ||
|
||
### What it does | ||
Checks for `__init__.py` methods that turned into generators | ||
via the presence of `yield` or `yield from` statements. | ||
|
||
### Why is this bad? | ||
Generators are not allowed in `__init__.py` methods. | ||
|
||
### Example | ||
```python | ||
class Foo: | ||
def __init__(self): | ||
yield 1 | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's
__init__
methods ... not__init__.py
methods (you made the mistake twice).