Skip to content
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-comprehensions (C4) - Remove unnecessary list comprehensions in additional contexts that can take arbitrary iterables (C419) #12648

Closed
gandhis1 opened this issue Aug 2, 2024 · 9 comments · Fixed by #12657
Labels
good first issue Good for newcomers help wanted Contributions especially welcome rule Implementing or modifying a lint rule

Comments

@gandhis1
Copy link

gandhis1 commented Aug 2, 2024

The following code all creates unnecessary list comprehensions

    a = all([i is not None for i in range(10)])
    b = tuple([i * 2 for i in range(10)])
    c = sum([i * 2 for i in range(10)])
    d = ",".join([str(i * 2) for i in range(10)])

ruff will only suggest changes on the first line:

stuff.py:3:13: C419 Unnecessary list comprehension
  |
2 | def main() -> None:
3 |     a = all([i is not None for i in range(10)])
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C419
4 |     b = tuple([i * 2 for i in range(10)])
5 |     c = sum([i * 2 for i in range(10)])
  |
  = help: Remove unnecessary list comprehension

However all of these are unnecessary list comprehensions. It ought to be able to identify them all. For one, the docs suggest that sum is covered, but from the above, it appears to not fully be.

@charliermarsh
Copy link
Member

sum is covered under --preview.

@charliermarsh
Copy link
Member

Honestly not sure why tuple doesn't have any corresponding rules.

@charliermarsh
Copy link
Member

Oh, I think it's because tuple([1, 2]) gets rewritten to (1, 2) (so the rule only supports literals). We could extend it to remove the inner list.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Aug 2, 2024
@charliermarsh
Copy link
Member

I think we should expand C409 to include comprehensions.

@charliermarsh charliermarsh added good first issue Good for newcomers help wanted Contributions especially welcome labels Aug 2, 2024
@charliermarsh
Copy link
Member

C419 is an example of how that logic would work -- it's nearly identical. (This would be behind --preview for now.)

@eth3lbert
Copy link
Contributor

While tuple and "".join can be rewritten without list comprehensions, it doesn't necessarily mean there's a performance improvement.

In [1]: %timeit tuple([i for i in range(1000)])
20.4 μs ± 148 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

In [2]: %timeit tuple(i for i in range(1000))
39.7 μs ± 205 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

In [3]: %timeit ",".join([str(i * 2) for i in range(10)])
1.02 μs ± 22.6 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [4]: %timeit ",".join(str(i * 2) for i in range(10))
1.49 μs ± 14.4 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

@charliermarsh
Copy link
Member

I believe the same is true for sum et al which is why they're still in preview and we considered adding a separate rule for them.

@dylwil3
Copy link
Collaborator

dylwil3 commented Aug 4, 2024

It seems like a toss up to me whether it should be C409 or C419 that should be expanded to include this violation/fix. In the PR above I did what was suggested above (and extended C409) - but let me know if you have second thoughts and I can extend C419 instead.

@gandhis1
Copy link
Author

gandhis1 commented Aug 5, 2024

What about str.join?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Contributions especially welcome rule Implementing or modifying a lint rule
Projects
None yet
4 participants