-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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-pyi] Implement PYI057 #11486
Conversation
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
PYI057 | 1 | 1 | 0 | 0 | 0 |
Hi! The primary reviewer for this is out right now so it might be a bit slower than usual to get reviewed. Could you change the rule to be in Additionally, I think use of ruff/crates/ruff_python_semantic/src/model.rs Lines 726 to 741 in f79c980
|
Welcome to the project :) |
I thought there might be something like this already, but couldn't find it. Thanks! |
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.
Great to see you around here @tomasr8! :-D Overall this looks great.
crates/ruff_linter/src/rules/flake8_pyi/rules/bytestring_usage.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/bytestring_usage.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/bytestring_usage.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/bytestring_usage.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Alex Waygood <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
oops didn't notice you already fixed the conflict 😅 |
hehe, no worries |
Thanks for the review! (as always :D) I removed the The |
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 very much! Looks great now. I just pushed a couple of nitpick-fixes to your branch :-)
let semantic = checker.semantic(); | ||
if !semantic | ||
.seen | ||
.intersects(Modules::TYPING | Modules::COLLECTIONS) | ||
{ | ||
return; | ||
} |
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.
@tomasr8: I added this in my latest push. It's an optimisation we do in a lot of rules. It's a very cheap check to see whether any of the relevant modules we care about have been imported at all. If not, we can just short-circuit the rest of the rule :-)
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.
Ah cool! I'll keep that in mind for future PRs :)
Summary
Adds Y057 from flake8-pyi.
I don't think we can apply any autofix here?
I'm new to rust so apologies in advance if I've made some rookie mistakes 😅
Test Plan
cargo test
+cargo insta review