Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Defaults should really be defaults #469

Closed
joaotavora opened this issue Dec 7, 2018 · 10 comments · Fixed by #1949
Closed

Defaults should really be defaults #469

joaotavora opened this issue Dec 7, 2018 · 10 comments · Fixed by #1949
Labels
protocol LSP compliance

Comments

@joaotavora
Copy link

I'm developing Eglot, an LSP client for emacs. If the following command

dotnet   path/to/output/bin/Release/Microsoft.Python.LanguageServer.dll

is used to start the server , then this :initializationOptions is enough to get a minimal working server.

{"interpreter":{"properties":{"UseDefaultDatabase":true}}}

If, on the other hand, :initializationOptions is empty or missing from the :initialize request, the server errors immediately with

{"code":-32000,
"message":"Object reference not set to an instance of an object.",
"data":"   at Microsoft.Python.LanguageServer.Implementation.LanguageServer.Initialize(JToken token, CancellationToken cancellationToken) in /home/capitaomorte/Source/Csharp/python-language-server/src/LanguageServer/Impl/LanguageServer.Lifetime.cs:line 41"}

My question here is this: if the "database", whatever it means in your server, is the "default", shouldn't it be used when the user defaults on it, i.e. doesn't provide it?

It would be nice if the server, much like Palantir's pyls, could:

  • use the best possible defaults
  • deduce the remaining settings from its environment and
  • use :initializationOptions for allowing user/client tweaks to those settings.
@MikhailArkhipov
Copy link

MikhailArkhipov commented Dec 9, 2018

DatabasePath must be provided - it is where LS caches information on compiled modules. UseDefaultDatabase applies to VS which ships certain modules pre-cached. Options are indeed poorly documented, we should be fixing this. Atm you can look intohttps://github.com/Microsoft/vscode-python/blob/master/src/client/activation/languageServer/languageServer.ts on what is set in the initialization.

Also, LS has no facilities to locate Python interpreter executable or determine search paths. This information is provided by the client app. Same for typeshed paths or exclusions.

@joaotavora
Copy link
Author

DatabasePath must be provided - it is where LS caches information on compiled modules

But I didn't provide it and it started: shouldn't it error out? More importantly, what's the matter with defaulting it to something like the current working directory + .mspylsdb. That wouldn't hurt VS at all, and it would help server-agnostic clients like Eglot.

@joaotavora
Copy link
Author

Also, LS has no facilities to locate Python interpreter executable or determine search paths.

Do you mean it currently doesn't have those facilities or that it's particularly difficult in C# to search PATH for python or python.exe and parse the output of --version?

@jakebailey
Copy link
Member

For all of the above, Mikhail is just describing the current behavior; not necessarily what the correct or future behavior is/should be.

If the database path is not provided, it should probably error out if we don't provide a default, yeah. Certainly not crash with a nullref.

The current implementation does not do any sort of searching for Python or figuring out what should be used; it just accepts the info given to it by the editor (VS and VSC have environment selectors and extra options for search paths, etc). We technically have some amount of logic that does searching, but it's only used during testing to find the multiple versions of Python we have installed for testing.

@joaotavora
Copy link
Author

Thanks @jakebailey and @MikhailArkhipov

I understand the situation, I just thought of reporting this as the perspective of some non-VS LSP client. I understand your server is quite new: if you fix these (presumably simple) things, I can add your server to my client's "try this server" list and get more users to try it out, thus helping you (again, presumably) develop a better server.

@MikhailArkhipov
Copy link

The LS is work in progress, it is in 'preview' in VS Code. The are significant changes coming, see #425 and #471. The database option most probably will be gone in a few weeks. Currently most, if not all, of our capacity is on VS Code. We are yet to update VS 2019 with the new code, #32 is still in progress.

When are ready to make LS available for general consumption, we will definitely perform testing with other clients. Meanwhile we may fix things that are simple but then postpone others to the general availability phase.

@MikhailArkhipov
Copy link

@jakebailey - is this covered now? We've made a number of changes since.

@jakebailey
Copy link
Member

jakebailey commented Sep 18, 2019

Not quite. If you send nothing, things will still break because the interpreter used is undefined (and likely a few other things; the typeshed location comes to mind).

@rwols
Copy link

rwols commented Mar 18, 2020

initializationOptions, as well as settings, must be set right now in order to not crash this server. See: sublimelsp/LSP#918

My advice: if there's no interpreterPath set, try some common paths like /usr/bin/python3.
If there are no settings set, use common defaults too.

@jakebailey
Copy link
Member

Related: #1938

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
protocol LSP compliance
Projects
None yet
4 participants