Closed ghost closed 9 months ago
ghost
commented
10 months ago I think it might be better if users can configure the severity as in python-lsp-ruff, but no builtin plugins support that feature so I am not sure how we are going to do it.
Anyway, with this PR at least the flake8 plugin reports the same severity levels as pyflakes, mccabe, and pycodestyle, which hopefully improves the situation a bit.
ghost
commented
10 months ago Hi @ccordoba12, I prepared a new version which generates the set of error codes dynamically as you suggested. However, as in this comment, there are also some (maybe minor) issues with this approach, so I am not sure if I should overwrite the previous version. Therefore, I instead attach the patch here:
From 4c4ea33ef2a2ccdf11e3189b8a324c8c2d7ecf11 Mon Sep 17 00:00:00 2001
From: Pengji Zhang <me@pengjiz.com>
Date: Sun, 19 Nov 2023 23:07:24 -0500
Subject: [PATCH] Refine diagnostic severity for flake8
Previously all diagnostics that starts with "F" (generated from pyflakes)
are reported as errors, which could be too high. As such, in this commit
we change to report only certain pyflakes diagnostics as errors and all
the others as warnings (aligned with the `pyflakes_lint` plugin).
Fix #256.
---
pylsp/plugins/flake8_lint.py | 13 ++++++++++++-
test/plugins/test_flake8_lint.py | 8 ++++----
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/pylsp/plugins/flake8_lint.py b/pylsp/plugins/flake8_lint.py
index 8d8d4c5..654dc20 100644
--- a/pylsp/plugins/flake8_lint.py
+++ b/pylsp/plugins/flake8_lint.py
@@ -9,7 +9,10 @@ import sys
from pathlib import PurePath
from subprocess import PIPE, Popen
+from flake8.plugins.pyflakes import FLAKE8_PYFLAKES_CODES
+
from pylsp import hookimpl, lsp
+from pylsp.plugins.pyflakes_lint import PYFLAKES_ERROR_MESSAGES
log = logging.getLogger(__name__)
FIX_IGNORES_RE = re.compile(r"([^a-zA-Z0-9_,]*;.*(\W+||$))")
@@ -20,6 +23,14 @@ UNNECESSITY_CODES = {
"F523", # .format(...) unused positional arguments
"F841", # local variable `name` is assigned to but never used
}
+# NOTE: If the user sets the flake8 executable with workspace configuration, the
+# error codes in this set may be inaccurate.
+ERROR_CODES = (
+ # Errors from the pyflakes plugin of flake8
+ {FLAKE8_PYFLAKES_CODES.get(m.__name__, "E999") for m in PYFLAKES_ERROR_MESSAGES}
+ # Syntax error from flake8 itself
+ | {"E999"}
+)
@hookimpl
@@ -208,7 +219,7 @@ def parse_stdout(source, stdout):
# show also the code in message
msg = code + " " + msg
severity = lsp.DiagnosticSeverity.Warning
- if code == "E999" or code[0] == "F":
+ if code in ERROR_CODES:
severity = lsp.DiagnosticSeverity.Error
diagnostic = {
"source": "flake8",
diff --git a/test/plugins/test_flake8_lint.py b/test/plugins/test_flake8_lint.py
index 882bc99..c2d711e 100644
--- a/test/plugins/test_flake8_lint.py
+++ b/test/plugins/test_flake8_lint.py
@@ -40,7 +40,7 @@ def test_flake8_unsaved(workspace):
assert unused_var["code"] == "F841"
assert unused_var["range"]["start"] == {"line": 5, "character": 1}
assert unused_var["range"]["end"] == {"line": 5, "character": 11}
- assert unused_var["severity"] == lsp.DiagnosticSeverity.Error
+ assert unused_var["severity"] == lsp.DiagnosticSeverity.Warning
assert unused_var["tags"] == [lsp.DiagnosticTag.Unnecessary]
@@ -55,7 +55,7 @@ def test_flake8_lint(workspace):
assert unused_var["code"] == "F841"
assert unused_var["range"]["start"] == {"line": 5, "character": 1}
assert unused_var["range"]["end"] == {"line": 5, "character": 11}
- assert unused_var["severity"] == lsp.DiagnosticSeverity.Error
+ assert unused_var["severity"] == lsp.DiagnosticSeverity.Warning
finally:
os.remove(name)
@@ -101,7 +101,7 @@ def test_flake8_respecting_configuration(workspace):
"end": {"line": 5, "character": 11},
},
"message": "F841 local variable 'a' is assigned to but never used",
- "severity": 1,
+ "severity": 2,
"tags": [1],
},
]
@@ -116,7 +116,7 @@ def test_flake8_respecting_configuration(workspace):
"end": {"line": 0, "character": 9},
},
"message": "F401 'os' imported but unused",
- "severity": 1,
+ "severity": 2,
"tags": [1],
}
]
--
2.30.2Please let me know what you think about this patch (and my concerns). Thank you!
ccordoba12
commented
9 months ago I think it's better if we generate the error codes dynamically, as in patch you posted above. If we don't do that, I'm afraid it could easily diverge from the Pyflakes codes in the future.
In addition, we have an upper cap on Flake8
So, it shouldn't be hard to detect when the import required for that changes.
ccordoba12
commented
9 months ago @kunhtkun, could you apply your patch above so we can merge this one? If I don't hear from you in 6 hours or so, I'll do it myself so we can include your work in our next release.
ghost
commented
9 months ago Thank you!
(Sorry for the late reply. I was on a vacation.)
On Mon, Dec 18, 2023 at 5:19 PM Carlos Cordoba @.***> wrote:
@ccordoba12 approved this pull request.
Thanks @kunhtkun for your work on this!
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
Previously all diagnostics that starts with "F" (generated from pyflakes) are reported as errors, which could be too high. As such, in this commit we change to report only certain pyflakes diagnostics as errors and all the others as warnings. The current list of error codes is in sync with the
pyflakes_lintplugin.While it is possible to generate such a list (or more precisely, a
set) during runtime (see the snippet in the comment), that might be less ideal because:flake8as an executable and parse its outputs, so using theflake8module in Python may bring inconsistency (e.g., the user install Flake8 in a different virtual environment from pylsp).So it seems better to maintain such a list of error codes manually.
Fix #256.