python-lsp / python-lsp-server

Fork of the python-language-server project, maintained by the Spyder IDE team and the community
MIT License
1.87k stars 195 forks source link

flake8 test failure due to obsolete flake8 configuration #286

Open gav451 opened 1 year ago

gav451 commented 1 year ago

Hello, I traced failing test report below

=========================== short test summary info ============================
FAILED test/plugins/test_flake8_lint.py::test_flake8_multiline - AssertionError: assert ['flake8', '-...W3,W6,B,B950'] == ['flake8', '-...h/...
============ 1 failed, 136 passed, 9 skipped, 7 warnings in 48.28s =============

back to the presence of a ~/.config/flake8 file that is picked up by pylsp/config/flake8_conf.py. The patch below makes the test succeed because it excludes extra options picked up from the configuration file.

--- a/test/plugins/test_flake8_lint.py
+++ b/test/plugins/test_flake8_lint.py
@@ -126,7 +126,7 @@ exclude =
         flake8_lint.pylsp_lint(workspace, doc)

     call_args = popen_mock.call_args[0][0]
-    assert call_args == ["flake8", "-", "--exclude=blah/,file_2.py"]
+    assert call_args[3] == ["flake8", "-", "--exclude=blah/,file_2.py"]

     os.unlink(os.path.join(workspace.root_path, "setup.cfg"))

However, according to the documentation section Flake8 Configuration Locations, flake8 looks for its configuration files (setup.cfg, tox.ini, and .flake8) only in its project or workspace directory. I would expect that pylsp would do the same to ensure identical flake8 messages from the command line and the server. Do you think it is worthwhile to propose a PR implementing this policy?

ccordoba12 commented 1 year ago

So is there not a global flake8 configuration? Or is it not recommended?

gav451 commented 1 year ago

No, contrary to pycodestyle, flake8 has no global configuration file (its removal gave rise to a lot of discussion, but now I think it was a wise decision). The simple function at https://github.com/PyCQA/flake8/blob/ff8988bd586a4af192390e4316772333f34ba1e5/src/flake8/options/config.py#L55 implements the configuration file policy of flake8.

Since, the flake8 design and coding look really clean, the PR may be as simple as importing this function into flake8_conf.py to delegate the handling of this policy to flake8.

PS: Although I have succeeded in configuring my client for the pylsp flake8 plugin, I am still using pyls_flake8. The most important reason for this is the non-compliance with the configuration file policy of the flake8 plugin. However, pyls_flake8 is unmaintained and I like to stop using it.

ccordoba12 commented 1 year ago

No, contrary to pycodestyle, flake8 has no global configuration file

Ok, I see.

Since, the flake8 design and coding look really clean, the PR may be as simple as importing this function into flake8_conf.py to delegate the handling of this policy to flake8.

Sounds good to me. Please go ahead with the proposed changes then.