-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[flake8-bugbear
] Implement return-in-generator
(B901
)
#11644
[flake8-bugbear
] Implement return-in-generator
(B901
)
#11644
Conversation
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
B901 | 2 | 2 | 0 | 0 | 0 |
crates/ruff_linter/src/rules/flake8_bugbear/rules/return_x_in_generator.rs
Outdated
Show resolved
Hide resolved
} | ||
} | ||
_ => { | ||
self.in_expr_statement = stmt.is_expr_stmt(); |
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.
I think I would do this by checking Stmt::Expr
here, and then just introspecting the value directly to see if the value is Expr::Yield
.
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.
Good idea, I updated the implementation to this.
Thanks! Two small comments based on your questions. |
b88f550
to
a335dea
Compare
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.
Thanks!
flake8-bugbear
] Implement return-x-in-generator
(B901
)flake8-bugbear
] Implement return-in-generator
(B901
)
a335dea
to
253e157
Compare
Summary
This PR implements the rule B901, which is part of the opinionated rules of
flake8-bugbear
.This rule seems to be desired in
ruff
as per #3758 and #2954 (comment).Test Plan
As this PR was made closely following the CONTRIBUTING.md, it tests using the snapshot approach, that is described there.
Sources
The implementation is inspired by the original implementation in the
flake8-bugbear
repository. The error message and test file where also copied from there.The documentation I came up with on my own and needs improvement. Maybe the example given in #2954 (comment) could be used, but maybe they are too complex, I'm not sure.
Open Questions
Documentation. (See above.)
Can I access the parent in a visitor?
The original implementation references the
yield
statement's parent to check if it is an expression statement. I didn't find a way to do this inruff
and used theis_expresssion_statement
field on the visitor instead. What are your thoughts on this? Is it possible and / or desired to access the parent node here?Is
Option::is_some(...)
->...unwrap()
the right thing to do?Referring to this piece of code. From my understanding, the
.unwrap()
is safe, because it is checked thatreturn_
is notNone
. However, I feel like I missed a more elegant solution that does both in one.Other
I don't know a lot about this rule, I just implemented it because I found it in a good first issueGood for newcomers
.
I'm new to Rust, so any constructive critisism is appreciated.