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] Skip C416 if comprehension contains unpacking #14909

Merged
merged 8 commits into from
Dec 20, 2024

Conversation

harupy
Copy link
Contributor

@harupy harupy commented Dec 11, 2024

Summary

Fix #11482. Applies adamchainz/flake8-comprehensions#205 to ruff. C416 should be skipped if comprehension contains unpacking. Here's an example:

list_of_lists = [[1, 2], [3, 4]]

# ruff suggests `list(list_of_lists)` here, but that would change the result.
# `list(list_of_lists)` is not `[(1, 2), (3, 4)]`
a = [(x, y) for x, y in list_of_lists]

# This is equivalent to `list(list_of_lists)`
b = [x for x in list_of_lists]

Test Plan

Existing checks

Copy link
Contributor

github-actions bot commented Dec 11, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood AlexWaygood added rule Implementing or modifying a lint rule bug Something isn't working labels Dec 11, 2024
@dylwil3 dylwil3 changed the title Skip C416 if comprehension contains unpacking [flake8-comprehensions] Skip C416 if comprehension contains unpacking Dec 13, 2024
@harupy
Copy link
Contributor Author

harupy commented Dec 17, 2024

@dylwil3 @dhruvmanila Thanks for the comments. Could you take another look?

|
9 | {k: v for k, v in d.items()}
10 | [(k, v) for k, v in d.items()]
11 | {k: (a, b) for k, (a, b) in d.items()}
Copy link
Contributor Author

@harupy harupy Dec 17, 2024

Choose a reason for hiding this comment

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

d might look like {"list": [1, 2]}. If so, it can't be fixed to dict(d.items()).

@dhruvmanila
Copy link
Member

Looking at this now

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Looks good. It's a bummer that we have to move away from ComparableExpr which adds a lot of complexity around matching and extracting the values.

@harupy harupy requested a review from dhruvmanila December 19, 2024 14:15
Signed-off-by: harupy <[email protected]>
@dylwil3
Copy link
Collaborator

dylwil3 commented Dec 19, 2024

Does this also solve #4491 ? Could you add a test demonstrating that if so?

@harupy
Copy link
Contributor Author

harupy commented Dec 20, 2024

@dylwil3 No, it doesn't solve that issue. To solve that issue, I think type inference is required.

@dhruvmanila dhruvmanila merged commit 6195c02 into astral-sh:main Dec 20, 2024
21 checks passed
@harupy harupy deleted the fix-C416-unpack branch December 20, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive for list comprehension
4 participants