Conflict between README and CONFIGURATION #263

Activity

1. krassowski commented on Sep 12, 2022

   

   krassowski

   on Sep 12, 2022

   Last edited by krassowski

   Member

   More actions

   Thanks for opening an issue. flake8 clearly should have been included (see test case in

   python-lsp-server/test/plugins/test_flake8_lint.py

   Line 99 in fa27a2b

   workspace.update_config({"pylsp": {"configurationSources": ["flake8"]}})

   and

   python-lsp-server/pylsp/config/config.py

   Line 53 in 98dc9c0

   self._config_sources['flake8'] = Flake8Config(self._root_path)

   It is however not clear to me if pyflakes should be there; there is no config source for those (unless it comes from a plugin):

   python-lsp-server/pylsp/config/config.py

   Lines 50 to 60 in 98dc9c0

   	self._config_sources = {}
   	try:
   	from .flake8_conf import Flake8Config
   	self._config_sources['flake8'] = Flake8Config(self._root_path)
   	except ImportError:
   	pass
   	try:
   	from .pycodestyle_conf import PyCodeStyleConfig
   	self._config_sources['pycodestyle'] = PyCodeStyleConfig(self._root_path)
   	except ImportError:
   	pass
2. doolio commented on Sep 14, 2022

   

   doolio

   on Sep 14, 2022

   Last edited by doolio

   ContributorAuthor

   More actions

   It is however not clear to me if pyflakes should be there; there is no config source for those (unless it comes from a plugin):

   True but maybe there should be considering flake8 is just a wrapper around pyflakes (which is really the error checker), pycodestyle and mccabe (plus others if also available such as pydocstyle (with flake8-docstrings), pep8-naming etc.).

   That is perhaps pylsp.configurationSources should accept all three possibilities:

   • ["pyflakes"] if you only want to check for code errors
   • ["pycodestyle"] if you only want to check for style errors
   • ["flake8"] if you want to check for both plus the added ability to check for complexity (via mccabe), docstrings (via pydocstyle + flake8-docstrings), PEP8 naming (via pep8-naming) etc.
3. ccordoba12

   added this to the v1.6.0 milestone on Sep 14, 2022
4. doolio commented on Sep 20, 2022

   

   doolio

   on Sep 20, 2022

   ContributorAuthor

   More actions

   Any further thoughts on this?
5. rchl commented on Sep 20, 2022

   

   rchl

   on Sep 20, 2022

   Last edited by rchl

   Contributor

   More actions

   I think the whole situation with configurationSources has derailed a bit.

   Here me out.
   It seems like introducing pylsp.configurationSources was a misguided decision (at least for flake8) as flake8 can and will read the relevant configuration files itself, regardless of that setting.

   The pylsp.configurationSources might have been introduced because someone realized that some of the configuration settings, specifically ones that depend on file path/name, do not work. That is because the files are linted through stdin and flake8 is not aware of actual file path.

   So someone added custom code to read the settings in flake8 pylsp plugin and apply those settings to linted files. So now we have a pylsp.configurationSources setting that is only really relevant for some of the settings but for other is not even needed.

   The proper solution IMO would be to just make flake8 aware of the actual file path by using the --stdin-display-name argument that it supports. It can be set to an actual path so that all the rules that work based on file path can work properly.

   (I might be missing something as I'm not a big user of flake8 but I was thinking about it yesterday)
6. ccordoba12 commented on Sep 25, 2022

   

   ccordoba12

   on Sep 25, 2022

   Member

   More actions

   The proper solution IMO would be to just make flake8 aware of the actual file path by using the --stdin-display-name argument that it supports. It can be set to an actual path so that all the rules that work based on file path can work properly.

   Thanks for analyzing the situation in depth @rchl. Could you create a pull request that implements your proposed solution?

   👍React with 👍1Reacted by Niall Dooley
7. ccordoba12

   closed this as completedin #267on Sep 27, 2022
8. rchl commented on Sep 27, 2022

   

   rchl

   on Sep 27, 2022

   Last edited by rchl

   Contributor

   More actions

   The proper solution IMO would be to just make flake8 aware of the actual file path by using the --stdin-display-name argument that it supports. It can be set to an actual path so that all the rules that work based on file path can work properly.

   Thanks for analyzing the situation in depth @rchl. Could you create a pull request that implements your proposed solution?

   Maybe I'd do that if I had more time but would still not be sure what extent of changes is expected here. Whether removing the flake8 form pylsp.configurationSources and all related code that manually reads the configuration would be OK or not.
9. ccordoba12 commented on Sep 28, 2022

   

   ccordoba12

   on Sep 28, 2022

   Member

   More actions

   Whether removing the flake8 form pylsp.configurationSources and all related code that manually reads the configuration would be OK or not.

   If it's not really necessary, then I'm +1 with doing that.