[flake8_comprehensions] add sum/min/max to unnecessary comprehension check (C419) #10759
Conversation
|
I'm confused about the docs parse error. |
I left in a stray unmatched bracket in a code example in the docs. I checked docs rendering locally, but I guess checking syntactic validity of docs code examples is a separate step I didn't run locally. Fixed now. |
|
Oh nice. Yay #10484 |
crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs
Outdated
Show resolved
Hide resolved
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| C419 | 41 | 41 | 0 | 0 | 0 |
|
I'd suggest looking through the ecosystem checks before merging. Let me know if you have any questions about them. |
|
I looked at all the new hits, they all look like valid and intended applications of the rule. Clearly this will cause new linter errors in stable on existing code though, not sure how we feel about that. I'm guessing in order to keep this in preview only we'd have to make it a new separate code? |
|
Ah yeah we should probably gate this with preview since it's a significant increase in scope. You don't need a separate code, you can just gate the additional calls with a check for preview. |
crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs
Outdated
Show resolved
Hide resolved
|
Ecosystem results look good (same as before, but now preview only; stable is unchanged); merging. |
|
Nice tool, but will there be an config option to match the old C419 behavior at some point? Right now, this is low-stake since it's enabled behind a preview-flag, but users may want to leave the old behavior if it results in a performance penalty of using a generator over a list comp. Would be good to leave it configurable or give it's own error code IMO. |
|
@Skylion007 Thanks for the feedback, it's a reasonable point that perhaps this should be two separate codes, since the tradeoffs look different for Would you be willing to open a new issue with a request for these to be two separate codes? Then we can take some time to collect more feedback (that's what preview is for, after all!) and make a decision before this goes stable. |
…check (C419) (astral-sh#10759) Fixes astral-sh#3259 ## Summary Renames `UnnecessaryComprehensionAnyAll` to `UnnecessaryComprehensionInCall` and extends the check to `sum`, `min`, and `max`, in addition to `any` and `all`. ## Test Plan Updated snapshot test. Built docs locally and verified the docs for this rule still render correctly.
Fixes #3259
Summary
Renames
UnnecessaryComprehensionAnyAlltoUnnecessaryComprehensionInCalland extends the check tosum,min, andmax, in addition toanyandall.Test Plan
Updated snapshot test.
Built docs locally and verified the docs for this rule still render correctly.