Skip to content

[flake8-comprehensions] Skip C416 if comprehension contains unpacking#14909

Merged
dhruvmanila merged 8 commits intoastral-sh:mainfrom
harupy:fix-C416-unpack
Dec 20, 2024
Merged

[flake8-comprehensions] Skip C416 if comprehension contains unpacking#14909
dhruvmanila merged 8 commits intoastral-sh:mainfrom
harupy:fix-C416-unpack

Conversation

@harupy
Copy link
Contributor

@harupy harupy commented Dec 11, 2024 •
edited by dylwil3
Loading

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 11, 2024 •
edited
Loading

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?

harupy
harupy commented Dec 17, 2024
View reviewed changes
...comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C416_C416.py.snap
|
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 •
edited
Loading

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

dhruvmanila commented Dec 19, 2024

Looking at this now

dhruvmanila
dhruvmanila approved these changes Dec 19, 2024
View reviewed changes
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
@harupy
Fix
7e67d05 
Signed-off-by: harupy <hkawamura0130@gmail.com>
@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 •
edited
Loading

@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
@harupy harupy deleted the fix-C416-unpack branch December 20, 2024 09:50
@BrewTestBot BrewTestBot mentioned this pull request Jan 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dylwil3 dylwil3 Awaiting requested review from dylwil3

@dhruvmanila dhruvmanila Awaiting requested review from dhruvmanila

Assignees

No one assigned

Labels

bug Something isn't working rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

False positive for list comprehension

4 participants

@harupy @dhruvmanila @dylwil3 @AlexWaygood