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

new rule: remove unnecessary list wrapping #2502

Closed
spaceone opened this issue Feb 2, 2023 · 7 comments
Closed

new rule: remove unnecessary list wrapping #2502

spaceone opened this issue Feb 2, 2023 · 7 comments
Labels
rule Implementing or modifying a lint rule

Comments

@spaceone
Copy link
Contributor

spaceone commented Feb 2, 2023

''.join([chr(i) for i in range(128)])
should be:
''.join(chr(i) for i in range(128))

@ngnpope
Copy link
Contributor

ngnpope commented Feb 2, 2023

This was discussed way back when for C407 in adamchainz/flake8-comprehensions#156.

It was rejected because using a list comprehension was faster, and still is:

# Python 3.10.9

In [1]: %timeit ",".join([str(i) for i in range(3)])
389 ns ± 1.79 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [2]: %timeit ",".join([str(i) for i in range(300)])
17.9 µs ± 90.9 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [3]: %timeit ",".join(str(i) for i in range(3))
473 ns ± 1.68 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [4]: %timeit ",".join(str(i) for i in range(300))
20.8 µs ± 21.4 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

# Python 3.11.1

In [1]: %timeit ",".join([str(i) for i in range(3)])
384 ns ± 4.26 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [2]: %timeit ",".join([str(i) for i in range(300)])
16.2 µs ± 169 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [3]: %timeit ",".join(str(i) for i in range(3))
501 ns ± 5.38 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [4]: %timeit ",".join(str(i) for i in range(300))
19.9 µs ± 268 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

@charliermarsh
Copy link
Member

So strange that that's faster! Intuitively I would've expected the opposite.

@spaceone
Copy link
Contributor Author

spaceone commented Feb 3, 2023

ah, I saw this in the past as well.

The reason:
adamchainz/flake8-comprehensions#156 (comment)

because join internally converts all generators to lists

But I think the performance aspect doesn't matter here, it's minimal in the recent python versions.
code readability is more important. If performance matters use rust instead of python :-P

So a new rule which can do this and be deactivated by folks who want the faster version.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Feb 3, 2023
@charliermarsh
Copy link
Member

I might err on the side of omitting if flake8-comprehensions intentionally omits it.

@charliermarsh
Copy link
Member

(Even though I personally wouldn't mind it.)

@ngnpope
Copy link
Contributor

ngnpope commented Feb 4, 2023

Out of interest, the performance dip for 3.11 in the third test (small generator) is likely caused by python/cpython#100762.

Even then, it'll still be slower. If we did implement this, it should be a different rule to C407. But it would be good to have a rule that enforces a list (comprehension) too given that's most optimal.

@charliermarsh
Copy link
Member

I'm going to close for now, we can re-open if flake8-comprehensions changes their stance.

@charliermarsh charliermarsh closed this as not planned Won't fix, can't repro, duplicate, stale May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

3 participants