Skip to content
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

Conflict between README and CONFIGURATION #263

Closed
doolio opened this issue Sep 12, 2022 · 7 comments · Fixed by #267
Closed

Conflict between README and CONFIGURATION #263

doolio opened this issue Sep 12, 2022 · 7 comments · Fixed by #267
Milestone

Comments

@doolio
Copy link
Contributor

doolio commented Sep 12, 2022

The README states when configuring flake8 one must

  1. Change the pylsp.configurationSources setting (in the value passed in from your client) to ['flake8'] in order to use the flake8 configuration instead.

However, the CONFIGURATION suggests the value is either ["pycodestyle"] or ["pyflakes"].

Which document is correct? I set it to ["flake8"] and it seems to work. Thanks for your time.

@krassowski
Copy link
Contributor

krassowski commented Sep 12, 2022

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

workspace.update_config({"pylsp": {"configurationSources": ["flake8"]}})
and
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):

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

@doolio
Copy link
Contributor Author

doolio commented Sep 14, 2022

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.

@ccordoba12 ccordoba12 added this to the v1.6.0 milestone Sep 14, 2022
@doolio
Copy link
Contributor Author

doolio commented Sep 20, 2022

Any further thoughts on this?

@rchl
Copy link
Contributor

rchl commented Sep 20, 2022

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)

@ccordoba12
Copy link
Member

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?

@rchl
Copy link
Contributor

rchl commented Sep 27, 2022

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.

@ccordoba12
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants