[refurb] Make fix unsafe if it deletes comments (FURB110)#22768
[refurb] Make fix unsafe if it deletes comments (FURB110)#22768ntBre merged 3 commits intoastral-sh:mainfrom
refurb] Make fix unsafe if it deletes comments (FURB110)#22768Conversation
|
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! I think the range check is too aggressive in some cases here. We should make sure comments are actually being removed before marking a fix unsafe.
crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or_operator.rs
Outdated
Show resolved
Hide resolved
| 17 | # Test for y. | ||
| 18 | y | ||
| 19 | ) | ||
| note: This is an unsafe fix and may change runtime behavior |
There was a problem hiding this comment.
I think the check may be too aggressive in this case, if I'm reading the diff correctly. It looks like the contents of the parentheses aren't actually modified here, so the comment is preserved.
There was a problem hiding this comment.
x, y = 1, 2
z = (
# 1
x) if ( # 2
x
) else (
# Test for y.
y
)In examples like these, there will also be deletions.
There was a problem hiding this comment.
I'm not sure what the right thing to do is in this case. The current rule may limit some cases, but at least there won't be any deletions in save fixes.
There was a problem hiding this comment.
Hmm, I guess you're right. I tried playing with this myself and couldn't find a way to avoid these cases without also making some comment deletions safe incorrectly.
…_operator.rs Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thank you! I'll merge this once today's release is finished.
2 participants
Summary
Test Plan