Support workspace/configuration #737
Conversation
| for requested_item in requested_items: | ||
| items.append(session.config.settings) | ||
|
|
||
| client.send_response(Response(request_id, items)) |
There was a problem hiding this comment.
Would it be a lot more work to do it "properly" from the start and expose a hook that LSP plugins (LanguageHandler implementations) could react to?
I don't know if it would be really worth it, as with static configuration it will probably be good enough for 99% of the cases, but just in case... :)
There was a problem hiding this comment.
Right, it would be nice if language handlers can replace this built-in implementation. Would it not be valuable for LSP to provide a functional example for other plugins to improve on (with better defaults for example) ?
There was a problem hiding this comment.
I guess you are right. Providing default with a way to override would be the best.
For Eslint server, response can include properties like workspaceFolder and workingDirectory. To provide those, we'd need to be able to override response in LanguageHandler but to be fair, I've been running without those two and things are generally working without them. So I'm not sure how important are those.
| items = [] # type: List[Any] | ||
| requested_items = params.get("items") or [] | ||
| for requested_item in requested_items: | ||
| items.append(session.config.settings) |
There was a problem hiding this comment.
If we will go with this hardcoded solution, maybe we should consider not using root settings but have a more specific property in settings for this case. It should be better in case there is a need later to provide some other settings also.
There was a problem hiding this comment.
I found no explanation that this configuration request expects a different data structure than the one sent by didChangeConfiguration. With your example, users would suddenly have to move their config into a new property just because the server “upgraded” to the workspace/configuration request.
There was a problem hiding this comment.
OK, I thought it's some arbitrary settings. Didn't know it's something more specific that is already sent with specific message. Then it makes sense to do it like that.
tomv564
force-pushed
the
workspace_configuration_request
branch
from
7919b5e to
9b66ab8
Compare
|
The use of "section" in VS code's eslint: Server sends a blank section, and assumes the Code extension only returns eslint config (see: https://github.com/microsoft/vscode-eslint/blob/master/client/src/extension.ts#L490) Hopefully this means Ignoring |
|
ScopeUri is probably best implemented once we have multiple workspaces. |
tomv564
changed the title
This is enough to get the eslint server to pull configuration from the client config's
settingsobject.The kotlin language server also used the request briefly (with section parameter set to "kotlin") but reverted to
workspace/didChangeConfiguration.Goal is to cover needs discussed in issue #699
Need to figure out how to handle:
section (eslint doesn't use it, but I guess we could treat them as keys under the client config settings object?)Lack of consistent use, we wait for more server to adopt.scopeUri (at least send None for URIs outside the workspace?)wait for multiple workspace supportdo we need to check server support? (we could sendThere is no server capability.workspace/didChangeConfigurationnotifications with no content, unclear if needed)