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

Avoid FURB113 autofix if comments are present #8494

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Conversation

dhruvmanila
Copy link
Member

This PR avoids creating the fix for FURB113 if there are comments in between the append calls.

fixes: #8105

@dhruvmanila dhruvmanila added the bug Something isn't working label Nov 5, 2023
Copy link
Contributor

github-actions bot commented Nov 5, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh
Copy link
Member

I'm a little torn on this because there's now no way to access this fix whereas with unsafe we at least allow the user to opt-in.

@charliermarsh
Copy link
Member

@zanieb - Do you have an opinion on this issue more broadly (of fixes that delete comments)? I could see us marking them as unsafe, or as display, or omitting them entirely as in this PR.

@tdulcet
Copy link

tdulcet commented Nov 5, 2023

(I am the author of the original issue.) If possible, I would prefer to just put the autofix below the comments, as it is otherwise a good fix as you said.

Another option could be to write the autofix over multiple lines keeping the comments in place, but I suspect that would be much more difficult to implement:

-nums.append(4)
+nums.extend((4,
# Critically important comment 1
-nums.append(5)
+5,
# Critically important comment 2
-nums.append(6)
+6))

@dhruvmanila
Copy link
Member Author

I'm a little torn on this because there's now no way to access this fix whereas with unsafe we at least allow the user to opt-in.

This seems to already be an unsafe fix. I'd actually then go with display only for this one.

Another option could be to write the autofix over multiple lines keeping the comments in place, but I suspect that would be much more difficult to implement:

Yeah, we'd probably need to use LibCST to construct the fix instead.

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Nov 9, 2023

I'm going forward with the current solution as it's consistent with various other rules (PT014, SIM117, SIM102, etc.) where comments doesn't generate the fix but feel free to suggest otherwise.

@dhruvmanila dhruvmanila enabled auto-merge (squash) November 9, 2023 03:08
@dhruvmanila dhruvmanila merged commit 4760af3 into main Nov 9, 2023
16 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/furb113 branch November 9, 2023 03:10
@charliermarsh
Copy link
Member

I feel like all of those should just be unsafe. I don't think we should prevent the user from accessing the fix due to comments -- they should just have to opt in. But I'd like @zanieb's opinion too. (Not asking for any changes right now.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FURB113 (repeated-append) autofix deletes comments
3 participants