Improve module loading path handling at completion, liinting, and so on #712
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your interest in palantir/python-language-server, @flying-foozy! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
Such extra paths will be needed also at spawning pylint process, in order to avoid unintentional "import-error" for libraries installed there. Then, which one is suitable ?
BTW, I noticed that 'Introduce config source for "pyls"' commit in this series is not correct enough. Therefore, please do not merge this pull request as it is, even if keeping |
Please use this one to only have one setting about it. |
ccordoba12
changed the title
flying-foozy
force-pushed
the
improve-path-handling
branch
from
86fa2a0
to
ef7c0fb
Compare
|
Oops, I overlooked test failures on Windows, sorry. |
At getting the value in nested settings dict, newly added
get_config_by_path() is more efficient and readable than "get with
empty dict" chain like below, which is sometimes used in current
implementation.
settings.get("foo", {}).get("bar", {}).get("baz")
Before this commit, 'setup.py' or 'pyproject.toml' should be created
to specify source root(s) other than the workspace root, even if such
file is not actually needed.
In order to explicitly specify source root(s) in the workspace, this
commit introduces pyls.source_roots configuration, and makes
workspace.source_roots() return its value if configured.
Path in pyls.source_roots is ignored, if it refers outside of the
workspace.
This configuration is also useful in the case that the workspace
consists of multiple (independent) services and a library shared by
them, for example. In such case, N + 1 source roots are listed in
pyls.source_roots configuration.
BTW, this commit puts some utilities into _util.py for reuse in the
future. Especially, os.path.commonprefix() for checking inside-ness in
find_parents() below should be replaced with is_inside_of(), because
the former returns unintentional value in some cases.
if not os.path.commonprefix((root, path)):
log.warning("Path %s not in %s", path, root)
return []
For example:
- commonprefix(('/foo', '/bar')) returns not false value but '/'
- commonprefix(('/foo', '/foobar')) returns not false value but '/foo'
This is useful to specify source roots in relative path, if pyls process might run in other than the root of the target workspace(s).
Before this commit, plugins.jedi.extra_paths configuration is used only at execution of jedi.Script in Document.jedi_script(). Therefore, at spawning a process for other plugins (e.g pylint), such extra paths are ignored. It might causes unintentional failure of finding out libraries installed into those locations, even though Jedi can find out. After this commit, just using Document.sys_path() instead of sys.path is enough path configuration for any plugins.
Before this commit, plugins.jedi.extra_paths can be configured only
via Language Server Protocol didChangeConfiguration method.
This is inconvenient, if:
- it is difficult to issue LSP didChangeConfiguration method with
per-workspace configuration
- such configuration should be persisted inside workspace
This commit reads plugins.jedi.extra_paths configuration also from
project-level configuration "setup.cfg" and "tox.ini".
This is the preparation for spawning pylint process with pyls specific customization in subsequent change. Almost all of spawn_pylint is cited from pylint/epylint.py in 6d818e3b84b853fe06f4199eb8408fe6dfe033be (the latest version on master branch at 2019-12-04) of pylint. BTW, this commit imports StringIO manually for compatibility between Python 2.x and 3.x instead of using "six" like recent pylint 1.9.x (supporting python 2.x), because requiring "six" only for StringIO seems over-kill.
For example, jedi respects VIRTUAL_ENV environment variable at finding out libraries. Therefore, (virtualenv) runtime for pyls/jedi can be separated from one for the target workspace. On the other hand, pylint does not respect VIRTUAL_ENV, and might cause unintentional "import-error" (E0401) for libraries installed in such virtualenv, even though jedi can recognize them. In order to make pylint respect libraries installed into extra paths, this commit uses Document.sys_path() instead of sys.path of current pyls process, at spawning pylint. At this commit, Document.sys_path() should respect source roots in the workspace, VIRTUAL_ENV, PYTHONPATH, and plugins.jedi.extra_paths configuration.
This commit describes about not only features added by previous commits but also some underlying specifications.
flying-foozy
force-pushed
the
improve-path-handling
branch
from
ef7c0fb
to
3715b28
Compare
|
I re-pushed revised version of this series. |
flying-foozy
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BTW, the 3rd commit in this series ("Introduce pyls.extraSysPath configuration") will cover pull request #382