Skip to content

[pyupgrade] Mark UP008 fix safe if no comments in range #18683

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

robsdedude
Copy link
Contributor

Summary

Mark UP008's fix safe if it won't delete comments.

Relevant Issues

Fixes: #18533

Copy link
Contributor

github-actions bot commented Jun 15, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@robsdedude robsdedude marked this pull request as ready for review June 15, 2025 13:50
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thanks! This looks right to me from a quick skim, but I think this will need to be gated behind preview since UP008 is a stable rule, as was done in #17644.

@ntBre ntBre added rule Implementing or modifying a lint rule preview Related to preview mode features labels Jun 15, 2025
@robsdedude robsdedude requested a review from ntBre June 15, 2025 19:09
@ntBre
Copy link
Contributor

ntBre commented Jun 16, 2025

Could you resolve the merge conflicts here? Hopefully they're not too much trouble.

@robsdedude
Copy link
Contributor Author

@ntBre Done. Wasn't super straight-forward but manageable. Can you take a look so we can get it merged soon-ish before more conflicts arise? Thanks 🙇

Copy link
Contributor

@ntBre ntBre 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, thanks! Just a couple of nits on the docs and another merge conflict, sorry!

Comment on lines +47 to +50
/// This rule's fix does not preserve comments.
///
/// In [preview], the fix is marked safe if no comments are attached to the arguments.
/// Else, the fix is marked unsafe.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I kind of preferred the old text for the first sentence/paragraph here. The second paragraph looks good, though.

I think you'll also need a reference link for [preview], at least that's what I saw in other rules with a quick grep.

Suggested change
/// This rule's fix does not preserve comments.
///
/// In [preview], the fix is marked safe if no comments are attached to the arguments.
/// Else, the fix is marked unsafe.
/// This rule's fix is marked as unsafe because removing the arguments from a call
/// may delete comments that are attached to the arguments.
///
/// In [preview], the fix is marked safe if no comments are present.
///
/// [preview]: https://docs.astral.sh/ruff/preview/

I guess we can do without the very last sentence if we bring back the old first bit.

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
Development

Successfully merging this pull request may close these issues.

UP008 fix safety documentation is questionable
2 participants