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
vscode-extensions.ms-python.python: 2018.12.1 -> 2019.6.22090 #55149
vscode-extensions.ms-python.python: 2018.12.1 -> 2019.6.22090 #55149
Conversation
Looks good, but isn't this broken as described in #53923? |
Just tested on a random python file, it is broken and requires patching for the Jedi language server.
|
e8a0da9
to
a6a7f0b
Compare
@eadwu Is this ready now? |
Doesn't look like it, though I'll test it again tomorrow. Still broken. |
a6a7f0b
to
b598023
Compare
b598023
to
98a36b9
Compare
98a36b9
to
aebaa9e
Compare
aebaa9e
to
928979f
Compare
If anyone wants this to be working again, I'm pretty sure if they pick up #53923 and supply the requested changes it should work. (i think) |
928979f
to
ca83625
Compare
Should've implemented the changes from that PR, will test later. |
Looks to be fine, at least the language server part, since tooltips are showing up. Though maybe because of my settings, I'm getting an error with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me this would be a perfectly fine first take.
It would be better were the language server part be moved out to its own file but even from the pt of view of its developers, this really has only been tested against vscode.
Tested the changes with a project of mine and it seems to work fine as first glance (test discovery, debugging, symbols, linting integration).
So Approved with Suggestions (Not yet).
UPDATE: It might be wise to wait and test this a little bit more before merging. With some more complex projects the language server seem to have trouble finding some modules when running pytest test discovery which are properly discovered when running pytest --collect-only
from the same environment. Seems related to the isort issue above.
UPDATE2: Alright, it seems that the extensions changed quite a bit in how it works with environments. Activating the environment by selecting the interpreter from the status bar did the trick. For some reason, when there is a single environment, vscode does not automatically activate this environment even though your interpreter setting is explicitly set to "python.pythonPath": "python"
. This change seems to have been persisted somewhere (most likely in one of the cache directory under ~/.config/Code
) as it did work out of the box after reloading vscode. I hope this cache won't stick with an old interpreter once I update my nix environment... Might be related to the following doc fragment:
-
environments-and-terminal-windows
However, launching VS Code from a shell in which a certain Python
environment is activated does not automatically activate that
environment in the default integrated terminal.
Might be of interest for other nix users:
-
manually-specify-an-interpreter
You can use an environment variable in the path setting using the
syntax ${env:VARIABLE}. For example, if you've created a variable
named PYTHON_INSTALL_LOC with a path to an interpreter, you can then
use the following setting value:"python.pythonPath": "${env:PYTHON_INSTALL_LOC}",
By using an environment variable, you can easily transfer a project
between operating systems where the paths are different, just be
sure to set the environment variable on the operating system first.
UPDATE 3: It would seem that this is not totally functional yet. When I attempt to use the "Microsoft Python Analysis Engine" instead of "Jedi" for intelliSense by setting "python.jediEnabled": false
, I get a "The Python Tool Server crashed 5 times in the last 3 minutes" error notification. and nothing works anymore. @eadwu : Did you test with "python.jediEnabled": false
?
}."${arch}"; | ||
|
||
# version is languageServerVersion in the package.json | ||
languageServer = extractNuGet rec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe what was meant by @worldofpeace in his review of the preceding incarnation of this PR was that it would be preferable that languageServer
be moved in its own file (as it can be used by other packages) and taken as input.
This would indeed remove most of the complexity as the added build inputs are to my understanding meant only for the download, extraction and automated patchelf-ing of the language server:
stdenv, fetchurl, extractNuGet, icu, curl, openssl, lttng-ust, autoPatchelfHook
The extractNuGet
utility could then be moved out of of the extension sub folder as well to be potentially used by others.
This is however merely a recommendation as it could indeed be done later as the need arises.
|
||
meta = with lib; { | ||
license = licenses.mit; | ||
maintainers = [ maintainers.jraygauthier ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to add yourself as a maintainer ;)
name = "Python-Language-Server"; | ||
version = "0.2.82"; | ||
|
||
src = fetchurl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@worldofpeace : My understanding as to why fetchNuGet
is not used is that there is no official standalone release yet as mentioned in Add installation instructions to README · Issue #953 · microsoft/python-language-server.
We do not provide official downloads for the installation yet (although download URLs are not secret but they can change). So it is recommended for clients to build their own. One of the reasons is that build we use is only tested with VS Code.
It is also not possible (yet) to build this be ourselves as xbuild is officially deprecated in favor of msbuild
(see Mono 5.0.0 Release Notes | Mono) and does not meet the minimal requirement of this project (tried it myself).
Adding support for msbuild
(even borrowed from debian) is an unfinished process:
- mono50 doesn't include msbuild · Issue #29817 · NixOS/nixpkgs
- msbuild: init at 15.6 by leo60228 · Pull Request #43680 · NixOS/nixpkgs
- msbuild: init at 15.8 by leo60228 · Pull Request #50842 · NixOS/nixpkgs
So to me, what's has been done here is fine for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep completely agree here. Perhaps a todo should be added though for when that's resolved.
ca83625
to
d4289bf
Compare
@eadwu / @worldofpeace : I have been using the extension with |
d4289bf
to
a8b31a1
Compare
It would be a good idea to open issues in nixpkgs for the known regressions. |
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)