-
Notifications
You must be signed in to change notification settings - Fork 223
Implement workspace/didChangeConfiguration and workspace/configuration
#3813
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
base: main
Are you sure you want to change the base?
Implement workspace/didChangeConfiguration and workspace/configuration
#3813
Conversation
How to use the Graphite Merge QueueAdd the label graphite-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
e3db658 to
21b3410
Compare
This adds support for the pull model of dynamic configuration changes, as described here: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration Upon receiving `workspace/didChangeConfiguration` message the Ruby LSP server sends `workspace/configuration` to the client. The client replies. This reply is different than other replies handled by Ruby LSP so far, as it does not include `method` and needs to be associated with the request via `id`. This required implementing a collection of server-sent requests, to be able to match. The `result` of the reply is not a hash, instead it's an array of hashes, so it needed to be handled separately as well. Technically, upon receiving `workspace/configuration` the server should check if it should register or unregister some capabilities. I intended to do that too, but it started to become messy and also I did not have a way to properly test it.
21b3410 to
aebdb8e
Compare
| options = { initializationOptions: message[:result]&.first } | ||
| messages_to_send = @global_state.apply_options(options) |
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.
This is a hack to reuse apply_options, which expects initializationOptions, but perhaps should be properly extracted to a method just accepting the options to apply.
Also, this just takes first hash from the reply's result, which works here, because we only asked for one configuration item, but technically is not exactly in line with the spec.
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.
Yeah, this method is coupling information that we get during initialization (like the workspace folder) with other settings. It would indeed be nice to split into two methods:
- One that handles information required by the spec (workspace folders, capabilities and so on)
- Another that handles only the Ruby LSP's specific settings (what gets passed as
initializationOptions)
Beyond just improving the design, invoking this twice may have weird consequences, like accidentally changing the negotiated encoding, so I don't think we can reuse it as is.
For example, if the editor and server negotiated UTF-8 initially, invoking this method without passing the capabilities -> general -> positionEncodings data will result in the server choosing UTF-16 (respecting the spec's default encoding), which would mean that editor and server would be trying to communicate using different encodings - leading to crashes, documents being in an invalid state and so on.
| private | ||
|
|
||
| #: (Hash[Symbol, untyped] message) -> void | ||
| def workspace_configuration_did_change(message) |
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.
My understanding based on the spec is that the parameters of workspace/didChangeConfiguration already include which settings are supposed to be changed.
Why do we need to make a request to the client? Could we use what we receive as parameters instead?
| unless @test_mode | ||
| while (message = @outgoing_queue.pop) | ||
| @global_state.synchronize { @writer.write(message.to_hash) } | ||
| @global_state.synchronize do |
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.
We already process one response for server->client requests here.
Are we able to reuse that and avoid changing the base server?
| options = { initializationOptions: message[:result]&.first } | ||
| messages_to_send = @global_state.apply_options(options) |
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.
Yeah, this method is coupling information that we get during initialization (like the workspace folder) with other settings. It would indeed be nice to split into two methods:
- One that handles information required by the spec (workspace folders, capabilities and so on)
- Another that handles only the Ruby LSP's specific settings (what gets passed as
initializationOptions)
Beyond just improving the design, invoking this twice may have weird consequences, like accidentally changing the negotiated encoding, so I don't think we can reuse it as is.
For example, if the editor and server negotiated UTF-8 initially, invoking this method without passing the capabilities -> general -> positionEncodings data will result in the server choosing UTF-16 (respecting the spec's default encoding), which would mean that editor and server would be trying to communicate using different encodings - leading to crashes, documents being in an invalid state and so on.
Motivation
Closes #3785
workspace/didChangeConfigurationandworkspace/configurationare part of the spec. They are used to dynamically change the configuration of the server after initialization. Witheglot, they are used to provide per-project configuration via.dir-locals.elfiles (see: https://www.gnu.org/software/emacs/manual/html_node/eglot/Project_002dspecific-configuration.html).Implementation
This adds support for the pull model of dynamic configuration changes, as described here: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration
Upon receiving
workspace/didChangeConfigurationmessage the Ruby LSP server sendsworkspace/configurationto the client. The client replies. This reply is different than other replies handled by Ruby LSP so far, as it does not includemethodand needs to be associated with the request viaid. This required implementing a collection of server-sent requests, to be able to match.The
resultof the reply is not a hash, instead it's an array of hashes, so it needed to be handled separately as well.Technically, upon receiving
workspace/configurationthe server should check if it should register or unregister some capabilities. I intended to do that too, but it started to become messy and also I did not have a way to properly test it, aseglotdoes not support dynamic registration of capabilities.Automated Tests
TBH it's a bit short on tests. I wasn't sure where/how to add more. I welcome all the suggestions in that area, because it feels a bit undertested.
Manual Tests
I was able to swap the formatter and linter from Rubocop to StandardRB in
eglotusing this.dir-locals.el: