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

Support initializationOptions to configure the server #459

Merged
merged 8 commits into from
Oct 20, 2023

Conversation

tkrabel-db
Copy link
Contributor

@tkrabel-db tkrabel-db commented Oct 12, 2023

What changes are proposed in this pull request?

Fixes #195.

The language server supports passing initializationOptions with the initialize message, but actually doesn't do anything with it. I think this was an oversight. This PR supports overriding configuration settings using the initializationOptions field.

How is this tested?

  • Added a unit test
  • Manually

from pylsp import IS_WIN


INITIALIZATION_OPTIONS = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this as an example as this is a real world example for when to override confs

@@ -98,6 +98,10 @@ def __init__(self, root_uri, init_opts, process_id, capabilities):
self._plugin_settings, plugin_conf
)

self._plugin_settings = _utils.merge_dicts(
self._plugin_settings, self._init_opts.get("pylsp", {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Language servers typically don't scope initialization settings to server name because, unlike the workspace settings, those are specific to the server itself and don't have any chance of conflicting with other settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy to address this, but it seems that there is lack of consensus on whether initializationOptions should be used for server configuration.

Copy link
Member

@ccordoba12 ccordoba12 Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pylsp is the options namespace used for this server, so I'm fine with using it here.

@krassowski
Copy link
Contributor

krassowski commented Oct 12, 2023

Some context:

  • as per spec initialization options are different from configuration options; however there is no agreement as to how they are different; there was an attempt to standardize it in microsoft/language-server-protocol@545cd69 but after discussion in lsp#567 it was reverted due to lack of consensus
  • this was previously discussed in Maybe use initializationOptions as additional source of settings #195 so it is not an omission - we should follow the discussion in there
  • one way that these two could be use used differently, and are used differently by some language server is following microsoft/language-server-protocol@545cd69: initialization options could include pythonVersion: string which is required during initialization but configuration options would have showWarnings: boolean which can be adjusted on runtime

@tkrabel-db
Copy link
Contributor Author

tkrabel-db commented Oct 13, 2023

@krassowski thanks for the context.

I have a clear use case in mind: enabling rope_autoimport without a config file. From the code and #195 I see that workspace/didChangeConfiguration is the supported way to achieve that, and I am happy to go with that approach.

I feel the discussion around initializationOptions is unfortunate. There is no consensus around how initializationOptionsshould be used, but many developers find it very useful to configure the server on initialize. I think so too.

As more and more language servers will run in cloud VMs and not on a local machine, these servers need to be configurable dynamically. workspace/configure is a valid general approach to handle server configuration at runtime, but using that to enable/disable some features is overkill, wasteful and will lead to bad user behaviour.

  • It's wasteful because you will end up turning off features that might have been enabled already on initialize, consuming resources unnecessarily (think e.g. rope_autoimport, which needs to index python modules; that problem becomes worse when the language server lives in an ephemeral environment like a cloud VM)
  • It will lead to bad user behaviour since the client will already send messages and potentially receive responses from features that the used didn't even want to use in the first place (e.g. getting ruff diagnostics instead of flake8 ones).
  • It is overkill because it will require extra logic to make sure the server is responsive while requesting workspace/configuration and while applying it (something that would be a no-brainer with initializeOptions as the server hasn't send a response to that yet, so the client keeps waiting)

@tkrabel-db
Copy link
Contributor Author

tkrabel-db commented Oct 13, 2023

Anyway, I moved my comment to #195, i.e. here

@ccordoba12 ccordoba12 changed the title Support initializationOptions Support initializationOptions to configure the server Oct 19, 2023
@ccordoba12 ccordoba12 added this to the v1.9.0 milestone Oct 19, 2023
@ccordoba12 ccordoba12 added the enhancement New feature or request label Oct 19, 2023
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one tiny suggestion for you @tkrabel-db.

test/test_configuration.py Outdated Show resolved Hide resolved
@tkrabel-db
Copy link
Contributor Author

@ccordoba12 thanks for supporting this feature! I addressed you comment and also refactored the unit test to use the new test util functions

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks @tkrabel-db!

@ccordoba12 ccordoba12 merged commit 1f415b5 into python-lsp:develop Oct 20, 2023
10 checks passed
staticf0x pushed a commit to staticf0x/python-lsp-server that referenced this pull request Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maybe use initializationOptions as additional source of settings
4 participants