Apply some minor changes to unnecessary-list-index-lookup
#8932
Conversation
| /// It is more succinct to use the variable for the value at the current index which is already in scope from the iterator. | ||
| /// When iterating over a list with `enumerate`, the current item is already | ||
| /// available alongside its index. Using the index to look up the item is | ||
| /// unnecessary. |
There was a problem hiding this comment.
I try to wrap these at 80 characters so that they render reasonably in the terminal.
| checker.diagnostics.push(diagnostic); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I prefer to put the rule implementations up top, and helper functions at the bottom. That way, each file reads as (1) diagnostic definition, then (2) rule body.
| } | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
I made this an associated method.
| false | ||
| } | ||
| _ => false, | ||
| }); |
There was a problem hiding this comment.
This is a repeat of is_assignment.
30d3a9b
to
e73e44d
Compare
| match call_path.as_slice() { | ||
| ["", "enumerate"] => (), | ||
| ["builtins", "enumerate"] => (), | ||
| _ => return None, | ||
| } |
There was a problem hiding this comment.
I rewrote this as:
// Check that the function is the `enumerate` builtin.
if !semantic
.resolve_call_path(func.as_ref())
.is_some_and(|call_path| matches!(call_path.as_slice(), ["builtins" | "", "enumerate"]))
{
return None;
}Note, however, that you could also do let call_path = checker.semantic().resolve_call_path(func.as_ref())? here, which is more idiomatic than let-else with a return None.
|
|
||
| /// PLR1736 | ||
| pub(crate) fn unnecessary_list_index_lookup_comprehension(checker: &mut Checker, expr: &Expr) { | ||
| match expr { |
There was a problem hiding this comment.
For a single branch match, I tend to prefer let-else or if-let.
| call_expr: &'a Expr, | ||
| tuple_expr: &'a Expr, | ||
| semantic: &SemanticModel, | ||
| ) -> Option<(&'a str, &'a str, &'a str)> { |
There was a problem hiding this comment.
Able to avoid the clone that was here before by using a lifetime.
e73e44d
to
1dabc98
Compare
|
I'll learn from all this, thanks! 😄 |
|
Of course! Sorry that it took it getting merged for me to actually prioritize reviewing, that's a bad habit 😬 |
|
Summary
I was late in reviewing this but found a few things I wanted to tweak. No functional changes.