[perflint] Fix comment duplication in fixes (PERF401, PERF403)#23729
Conversation
|
|
Thanks! I'll try to give this a look tomorrow. Is the related issue #18787? |
ntBre
left a comment
There was a problem hiding this comment.
Thank you! I wrote a couple of comments, but I don't think I feel strongly enough about them to change anything, so I'll go ahead and land this as-is.
| def f(): | ||
| # comment duplication in target (https://github.com/astral-sh/ruff/issues/18787) | ||
| original = list(range(10000)) | ||
| filtered = [] | ||
| for ( | ||
| i # comment | ||
| ) in original: | ||
| if i > 0: | ||
| filtered.append(i) No newline at end of file |
There was a problem hiding this comment.
Did you have to change the code to handle this case as well as the one above? I ask because these look very similar to me, and I'm wondering if we can drop one of the tests, but if they exercise different parts of the code, it's fine to keep both.
| for_stmt.range, | ||
| &[to_append.range(), for_stmt.iter.range()], | ||
| ); | ||
| let mut ranges_to_ignore = vec![ |
There was a problem hiding this comment.
It's a bit sad to allocate a Vec here, but I think it's okay. It would be a slightly larger refactor to make comment_strings_in_range take an iterator and too much duplication to do something like:
if let Some(test) = if_test {
&[to_append.range(), for_stmt.iter.range(), for_stmt.target.range(), test.range()]
} else {
&[to_append.range(), for_stmt.iter.range(), for_stmt.target.range()]
}Although now that I write that out, I don't mind it as much as I expected.
perflint] Fix comment duplication in comprehension fixes (PERF401, PERF403)
perflint] Fix comment duplication in comprehension fixes (PERF401, PERF403)perflint] Fix comment duplication in fixes (PERF401, PERF403)
@ntBre thank you for the review and the merge. |
Summary
Test Plan
Closes: #18787