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

Add known limitation to C416 with dictionaries #13627

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Add known limitation to C416 with dictionaries #13627

merged 2 commits into from
Oct 7, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Oct 4, 2024

Part of #13625

See also #13629

@zanieb zanieb added the documentation Improvements or additions to documentation label Oct 4, 2024
Copy link
Contributor

github-actions bot commented Oct 4, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@zanieb zanieb marked this pull request as draft October 4, 2024 19:07
@zanieb zanieb changed the title Add known limitation to C416 with tuple keys Add known limitation to C416 with dictionaries Oct 4, 2024
@zanieb zanieb marked this pull request as ready for review October 4, 2024 20:58
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nice!

Comment on lines 49 to 50
/// When the comprehension iterates over a sequence, the fix is correct. However, Ruff cannot
/// consistently infer if the iterable type is a sequence or a mapping.
Copy link
Member

Choose a reason for hiding this comment

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

It's not really just the autofix that's incorrect in these cases: the message and suggestion of the rule's diagnostic is also incorrect in these cases

Suggested change
/// When the comprehension iterates over a sequence, the fix is correct. However, Ruff cannot
/// consistently infer if the iterable type is a sequence or a mapping.
/// When the comprehension iterates over a sequence, the rule's suggestion is correct.
/// However, Ruff cannot consistently infer if the iterable type is a sequence or a mapping.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, it should be dict(iterable.keys()) right? I think the rule is correct and just the fix is wrong?

Copy link
Member

Choose a reason for hiding this comment

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

Oh. Yeah, great point :)

@zanieb
Copy link
Member Author

zanieb commented Oct 7, 2024

I think we should try to improve C416 before it's stabilized, so it's not wrong so often. The fix could be safe for non-dict comprehensions if we don't mind dropping some comments.

@zanieb zanieb enabled auto-merge (squash) October 7, 2024 16:07
@AlexWaygood
Copy link
Member

The policy I've been using for the minor releases I've been shepherding has been not to stabilise any rules that have significant open issues on the tracker

@zanieb zanieb merged commit fb90f5a into main Oct 7, 2024
18 checks passed
@zanieb zanieb deleted the zb/c416-unsafe branch October 7, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants