-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
ERA001: autofix removes valid comments that do not contain code or removes not everything #4845
Comments
I don't think we're in a position to improve this much, the logic itself is derived from eradicate, and I'd bet eradicate has the same or very similar behavior. Perhaps we should remove the autofix from this rule though. It seems correct to flag that there's some commented-out code here, but rely on users to remove it. Alternatively, we could only autofix if all lines in a comment block are identified as code. |
We could make this as a Suggested ( /// The fix may be what the user intended, but it is uncertain.
/// The fix should result in valid code if it is applied.
/// The fix can be applied with user opt-in. |
I like
Perhaps we could use |
Another example: it thinks the comment is code even though
|
I believe this is considered as a valid code ( params: int |
I am absolutely in favor of not automatically removing these comments in any situation. Yes, commented-out code is usually a mistake, but you really don't want to loose actual comments accidentally. Our example:
|
@bodograumann what is the benefit of enabling the ERA001 rule for you then? |
It will still warn me and usually correctly so, but I still need to decide myself whether a line needs to be removed. |
Was about to open a new issue, but I think this fits here: While testing out ruff, I ran into some false positives for Some examples: # Keep this list sorted alphabetically
# (it is used in example) # Check if the ip address in either:
# local (loopback) or internal (private) ##
# Client (consumers)
##
...
##
# Server (protocol)
## All trigger I had also hoped that code fragments that are part of a larger comment wouldn't trigger # XXX: Cannot use:
# self.example(foo, 'bar')
# because of issue #1337 # XXX: Example category has a duplicate 'code' and is omitted
# EXAMPLE_CATEGORY = '1.0', 'Example Category' # Note: Admin site instance should be passed when calling
# ExampleAdminView.as_view(site=site) # main categories themselves are not part of the return value of
# get_subcategories() This is all using I've already marked For now I'll just be working around this by rewording/reformatting these comments until they don't trigger anymore. |
Yeah the ERA001 heuristics are very prone to false positives, unfortunately. I think there's room for improvement, but it's not entirely clear how. def foo():
x = 1
# add to x
# x += 1
return x I think this should raise as an ERA001 violation but if we ignore comments that are part of a larger non-code context then we won't raise here. Maybe that's worth the false negative to reduce the false positives. |
This seems like a bug: A single parenthesis triggers echo "# )" | ruff --select ERA001,RUF100 --stdin-filename test.py
test.py:1:1: ERA001 [*] Found commented-out code Adding a echo "# ) # noqa: ERA001" | ruff --select ERA001,RUF100 --stdin-filename test.py
test.py:1:6: RUF100 [*] Unused `noqa` directive (unused: `ERA001`) |
@jaap3 - Funnily enough that specific case was reported recently and is fixed on |
Rebase of #5119 authored by @evanrittenhouse with additional refinements. ## Changes - Adds `--unsafe-fixes` / `--no-unsafe-fixes` flags to `ruff check` - Violations with unsafe fixes are not shown as fixable unless opted-in - Fix applicability is respected now - `Applicability::Never` fixes are no longer applied - `Applicability::Sometimes` fixes require opt-in - `Applicability::Always` fixes are unchanged - Hints for availability of `--unsafe-fixes` added to `ruff check` output ## Examples Check hints at hidden unsafe fixes ``` ❯ ruff check example.py --no-cache --select F601,W292 example.py:1:14: F601 Dictionary key literal `'a'` repeated example.py:2:15: W292 [*] No newline at end of file Found 2 errors. [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ``` We could add an indicator for which violations have hidden fixes in the future. Check treats unsafe fixes as applicable with opt-in ``` ❯ ruff check example.py --no-cache --select F601,W292 --unsafe-fixes example.py:1:14: F601 [*] Dictionary key literal `'a'` repeated example.py:2:15: W292 [*] No newline at end of file Found 2 errors. [*] 2 fixable with the --fix option. ``` Also can be enabled in the config file ``` ❯ cat ruff.toml unsafe-fixes = true ``` And opted-out per invocation ``` ❯ ruff check example.py --no-cache --select F601,W292 --no-unsafe-fixes example.py:1:14: F601 Dictionary key literal `'a'` repeated example.py:2:15: W292 [*] No newline at end of file Found 2 errors. [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ``` Diff does not include unsafe fixes ``` ❯ ruff check example.py --no-cache --select F601,W292 --diff --- example.py +++ example.py @@ -1,2 +1,2 @@ x = {'a': 1, 'a': 1} -print(('foo')) +print(('foo')) \ No newline at end of file Would fix 1 error. ``` Unless there is opt-in ``` ❯ ruff check example.py --no-cache --select F601,W292 --diff --unsafe-fixes --- example.py +++ example.py @@ -1,2 +1,2 @@ -x = {'a': 1} -print(('foo')) +x = {'a': 1, 'a': 1} +print(('foo')) \ No newline at end of file Would fix 2 errors. ``` #7790 will improve the diff messages following this pull request Similarly, `--fix` and `--fix-only` require the `--unsafe-fixes` flag to apply unsafe fixes. ## Related Replaces #5119 Closes #4185 Closes #7214 Closes #4845 Closes #3863 Addresses #6835 Addresses #7019 Needs follow-up #6962 Needs follow-up #4845 Needs follow-up #7436 Needs follow-up #7025 Needs follow-up #6434 Follow-up #7790 Follow-up #7792 --------- Co-authored-by: Evan Rittenhouse <evanrittenhouse@gmail.com>
Rebase of #5119 authored by @evanrittenhouse with additional refinements. ## Changes - Adds `--unsafe-fixes` / `--no-unsafe-fixes` flags to `ruff check` - Violations with unsafe fixes are not shown as fixable unless opted-in - Fix applicability is respected now - `Applicability::Never` fixes are no longer applied - `Applicability::Sometimes` fixes require opt-in - `Applicability::Always` fixes are unchanged - Hints for availability of `--unsafe-fixes` added to `ruff check` output ## Examples Check hints at hidden unsafe fixes ``` ❯ ruff check example.py --no-cache --select F601,W292 example.py:1:14: F601 Dictionary key literal `'a'` repeated example.py:2:15: W292 [*] No newline at end of file Found 2 errors. [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ``` We could add an indicator for which violations have hidden fixes in the future. Check treats unsafe fixes as applicable with opt-in ``` ❯ ruff check example.py --no-cache --select F601,W292 --unsafe-fixes example.py:1:14: F601 [*] Dictionary key literal `'a'` repeated example.py:2:15: W292 [*] No newline at end of file Found 2 errors. [*] 2 fixable with the --fix option. ``` Also can be enabled in the config file ``` ❯ cat ruff.toml unsafe-fixes = true ``` And opted-out per invocation ``` ❯ ruff check example.py --no-cache --select F601,W292 --no-unsafe-fixes example.py:1:14: F601 Dictionary key literal `'a'` repeated example.py:2:15: W292 [*] No newline at end of file Found 2 errors. [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ``` Diff does not include unsafe fixes ``` ❯ ruff check example.py --no-cache --select F601,W292 --diff --- example.py +++ example.py @@ -1,2 +1,2 @@ x = {'a': 1, 'a': 1} -print(('foo')) +print(('foo')) \ No newline at end of file Would fix 1 error. ``` Unless there is opt-in ``` ❯ ruff check example.py --no-cache --select F601,W292 --diff --unsafe-fixes --- example.py +++ example.py @@ -1,2 +1,2 @@ -x = {'a': 1} -print(('foo')) +x = {'a': 1, 'a': 1} +print(('foo')) \ No newline at end of file Would fix 2 errors. ``` #7790 will improve the diff messages following this pull request Similarly, `--fix` and `--fix-only` require the `--unsafe-fixes` flag to apply unsafe fixes. ## Related Replaces #5119 Closes #4185 Closes #7214 Closes #4845 Closes #3863 Addresses #6835 Addresses #7019 Needs follow-up #6962 Needs follow-up #4845 Needs follow-up #7436 Needs follow-up #7025 Needs follow-up #6434 Follow-up #7790 Follow-up #7792 --------- Co-authored-by: Evan Rittenhouse <evanrittenhouse@gmail.com>
Is there scope for a PR for better algorithms, and a better set of false positive rules here? It will deviate from the eradicate plugin, would that be okay or should it be a different rule entirely then? |
Yeah definitely fine to fold in improvements even if it deviates from eradicate. |
Self-documenting here for later. Examples of false negatives: # if foo:
# elif bar:
# if (
# x
# ):
# else: Examples of false positives: # continue
# pragma: cover
# (something)
# O(1)
# this is O(1) |
Example:
Ruff output:
Contents of the file after fix:
The text was updated successfully, but these errors were encountered: