astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
30.71k stars 1.02k forks source link

`RUF100` should probably delete an entire comment `noqa` is part of #12251

Closed yury-fedotov closed 1 week ago

yury-fedotov commented 1 month ago

I think it applies to all error codes, but let me give an example where I encountered that.

Consider:

data = do_some_transformations()
return data

Which is a violation of RET504 - unnecessary assignment.

I needed to perform the assignment and return data, not return do_some_transformations(), so added a noqa: RET504 with explanation of why I want to bypass the rule:

data = do_some_transformations()
return data  # noqa: RET504 - to be able to debug the returned object.

Then I disabled RET504 in ruff config completely, and this noqa became unused. Here is where RUF100 comes into play, as it complains on unused error codes, and provides a fix.

The fix was:

-    return data  # noqa: RET504 - to be able to debug the returned object
+    return data  # - to be able to debug the returned object

While I believe it should have been:

-    return data  # noqa: RET504 - to be able to debug the returned object
+    return data

As it's probably quite safe to assume that entire content of the comment noqa is part of is explaining why a rule should by bypassed.

charliermarsh commented 1 month ago

I'm unsure -- I think the current behavior is intentional. But either seems reasonable.

MichaReiser commented 1 month ago

I think deleting the entire comment is fine, for as long as it doesn't delete anything coming after another #:

-    return data  # noqa: RET504 # fmt:skip
+    return data  # fmt: skip
charliermarsh commented 1 month ago

I agree!

charliermarsh commented 1 month ago

I think this is all here if anyone is interested in a good issue: https://github.com/astral-sh/ruff/blob/940df67823dc5237f95d36a94ef3a74dc4bd36fb/crates/ruff_linter/src/fix/edits.rs#L65

jeremeybingham commented 1 month ago

Is there a standard or a strong feeling here on preserving trailing whitespace following 'Preserved' comments? Meaning:

can: x = 1 # noqa # type: ignore and: x = 1 # noqa # type: ignore

both resolve to: x = 1 # type: ignore ?

or is there a conceivable edge case where that matters?

At any rate, I believe I've mostly got this but will spend some time tomorrow getting the testing framework and everything set up, should work regardless of the above, just changes the logic a bit to save those characters if we want 'em.

dhruvmanila commented 1 month ago

It should be fine to remove trailing whitespace but just a note that any other comments shouldn't be deleted (https://github.com/astral-sh/ruff/issues/12251#issuecomment-2226110486).

jeremeybingham commented 1 month ago

this was... much better than I expected the first pass to go, will have a PR on this once I figure out what's going on with these, but I'll take 4/2057 as a start!

Screenshot 2024-07-18 174653

dhruvmanila commented 1 month ago

@jeremeybingham Can you open the PR? I can help you debug these test failures.

jeremeybingham commented 1 month ago

will try asap, yes - have "fixed" my way into various fun new failure modes, and am now trying to consolidate the working bits of all that into something that compiles.