-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[flake8-bugbear] Add async-lru and aiocache decorators to the B019 rule checker #16450
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
base: main
Are you sure you want to change the base?
[flake8-bugbear] Add async-lru and aiocache decorators to the B019 rule checker #16450
Conversation
|
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.
Thank you.
I'm a bit hesitant to accept this change because it is for third party libraries. Most of our rules are limited to first-party modules or very popular first-party modules.
I'm not sure async-lru
meets that bar
/// - [don't lru_cache methods!](https://www.youtube.com/watch?v=sVjtp6tGo0g) | ||
#[derive(ViolationMetadata)] | ||
pub(crate) struct CachedInstanceMethod; | ||
pub(crate) struct CachedInstanceMethod { | ||
decorator_name: String, |
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.
Nit: I'd suggest using an enum here to avoid unnecessary String
allocation
#[derive(Copy, Clone, Deubg)]
enum LruDecorator {
FuncToolsLruCache,
FunctoolsCache,
AsyncLru,
AiocacheCached,
AiocacheCachedStampede,
AiocacheMultiCached
}
impl LruDecorator {
fn from_qualified_name(name: &[str]) -> Option<Self> {
match qualified_name.segments() {
["functools", "lru_cache"] => Some(Self::FunctoolsLruCache),
...
_ => None,
}
}
impl std::fmt::Display for LruDecorator {
fn fmt(f: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
Self::FuncToolsLruCache => f.write_str("functools.lru_cache"),
...
}
}
}
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.
Oh, really, thanks!
I used to think about allocation
/// Returns `Some(<decorator_name>)` if the given expression is one of the known | ||
/// cache decorators, otherwise `None`. | ||
fn get_cache_decorator_name(expr: &Expr, semantic: &SemanticModel) -> Option<String> { | ||
if let Some(qualified_name) = semantic.resolve_qualified_name(expr) { |
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.
We have to gate this new behavior behind preview mode because it's a considerable extension of the rule's intent (checker.settings().preview.is_enabled()
)
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 don't understand how to write insta tests in this case, could you explain it to me or give an example?
Hello! Here the download statistics for these two: It seems that |
88febe6
to
9a5d515
Compare
9a5d515
to
218755f
Compare
@zanieb, do you know how popular/recommended these libraries are in the Python ecosystem? E.g. are they as popular as |
Summary
Resolves #16436.
This change adds support for checking the
B019
rule against async cache decorators from the two most commonly used third-party libraries (aiocache and async-lru). It also updates the error message to clearly indicate which decorator was used.Test Plan
cargo nextest run
andcargo insta test
.