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] Account for list and set comprehensions in unnecessary-literal-within-tuple-call (C409) #12657

Merged
merged 9 commits into from
Aug 5, 2024

Conversation

dylwil3
Copy link
Contributor

@dylwil3 dylwil3 commented Aug 4, 2024

Summary

Make it a violation of C409 to call tuple with a list or set comprehension, and
implement the (unsafe) fix of calling the tuple with the underlying generator instead.

Closes #12648.

Test Plan

Test fixture updated, cargo test, docs checked for updated description.

Copy link

codspeed-hq bot commented Aug 4, 2024

CodSpeed Performance Report

Merging #12657 will not alter performance

Comparing dylwil3:tuple-comprehensions (6862cd3) with main (d6c6db5)

Summary

✅ 32 untouched benchmarks

Copy link
Contributor

github-actions bot commented Aug 4, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+7 -0 violations, +0 -0 fixes in 4 projects; 50 projects unchanged)

apache/airflow (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/providers/google/cloud/transfers/sql_to_gcs.py:259:41: C409 Unnecessary list comprehension passed to `tuple()` (rewrite as a generator)
+ dev/breeze/src/airflow_breeze/utils/packages.py:374:12: C409 Unnecessary list comprehension passed to `tuple()` (rewrite as a generator)
+ tests/_internals/capture_warnings.py:187:55: C409 Unnecessary list comprehension passed to `tuple()` (rewrite as a generator)
+ tests/always/test_example_dags.py:132:25: C409 Unnecessary list comprehension passed to `tuple()` (rewrite as a generator)

latchbio/latch (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ latch/functions/operators.py:106:27: C409 Unnecessary list comprehension passed to `tuple()` (rewrite as a generator)

mlflow/mlflow (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ tests/utils/test_uri.py:194:20: C409 Unnecessary list comprehension passed to `tuple()` (rewrite as a generator)

python/mypy (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ mypy/checkpattern.py:390:17: C409 Unnecessary list comprehension passed to `tuple()` (rewrite as a generator)

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
C409 7 7 0 0 0

@Skylion007
Copy link
Contributor

I'd argue this is only safe to flag for list comprehensions as set comprehensions will dedup and potentially reorder elements as well (not that you should rely on that nondeterm ordering but still).

@dylwil3
Copy link
Contributor Author

dylwil3 commented Aug 4, 2024

Good point! Another thing I noticed is that the suggestion text doesn't make sense in the context of comprehensions. I'll fix both next time I'm in front of a computer.

Thanks!

@dylwil3
Copy link
Contributor Author

dylwil3 commented Aug 5, 2024

Ok, should be good to go!

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! (FYI I had to move this behavior to preview to adhere to our versioning policy since it's an expansion in scope.)

@charliermarsh charliermarsh merged commit 25aabec into astral-sh:main Aug 5, 2024
19 checks passed
@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Aug 5, 2024
@dylwil3 dylwil3 deleted the tuple-comprehensions branch August 5, 2024 11:12
@hauntsaninja
Copy link
Contributor

hauntsaninja commented Aug 15, 2024

I find this kind of thing unfortunate, because tuple(i for i in range(10000)) is like 50% slower than tuple([i for i in range(10000)]).

There's no way to configure ruff to disambiguate between the two.
Edit: I should probably open a new issue #12912 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
4 participants