Refine diagnostic severity for flake8

[ghost]

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_lint plugin.

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:

• Flake8 does not guarantee a stable API.
• We run flake8 as an executable and parse its outputs, so using the flake8 module in Python may bring inconsistency (e.g., the user install Flake8 in a different virtual environment from pylsp).
• Doing so couples two plugins (flake8 and pyflakes) together.

So it seems better to maintain such a list of error codes manually.

Fix #256.

[ghost]

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.

[ccordoba12]

Hey @kunhtkun, thanks a lot for your contribution! I just left one suggestion for you, otherwise looks good to me.

[ccordoba12]

I suggest to use something similar to this code to keep Pyflakes and Flake8 error codes in sync without manual maintenance.

In other words, I suggest to build ERROR_CODES like this (with imports in the appropriate places, of course):

from flake8.plugins.pyflakes import FLAKE8_PYFLAKES_CODES

from pylsp.plugins.pyflakes_lint import PYFLAKES_ERROR_MESSAGES

ERROR_CODES = {FLAKE8_PYFLAKES_CODES[m.__name__] for m in PYFLAKES_ERROR_MESSAGES} | {"E999"}

[ghost]

Thanks for the review!

I was considering doing so but decided to maintain it manually because:

• Flake8 does not guarantee a stable API, so FLAKE8_PYFLAKES_CODES is somewhat internal.
• We run flake8 as an executable and parse its outputs, so using the flake8 module may bring import errors or inconsistencies (e.g., the user installs Flake8 in a different virtual environment from pylsp).

Honestly both are quite subtle though. If you think they are not problems I will change to what you suggest. Thank you!

[ghost]

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.2

Please let me know what you think about this patch (and my concerns). Thank you!

[ccordoba12]

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

python-lsp-server/pyproject.toml

Line 31 in 5f53f8e

"flake8>=6.1.0,<7",

So, it shouldn't be hard to detect when the import required for that changes.

[ccordoba12]

@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.

[ccordoba12]

Thanks @kunhtkun for your work on this!

[ghost]

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: ***@***.***>