Skip to content

Add known limitation to C416 with dictionaries#13627

Merged
zanieb merged 2 commits intomainfrom
zb/c416-unsafe
Oct 7, 2024
Merged

Add known limitation to C416 with dictionaries#13627
zanieb merged 2 commits intomainfrom
zb/c416-unsafe

Conversation

@zanieb
Copy link
Copy Markdown
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
@github-actions
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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