Added support for ILanguageServerConfiguration #207
Conversation
…aded from the client
|
@NTaylorMullen @mholo65 @tintoy @TylerLeonhardt any feedback you want to give before I push this through? 🌵 |
|
cc @rjmholt @david-driscoll this is great - we use our own (very basic) ConfigurationService and Configuration Handler today and I'm wondering how we might be able to use these changes. We keep track of "powershell" scoped settings and also a couple other generic ones ("files" and "search"). |
There was a problem hiding this comment.
Really cool stuff. @ajaybhargavb you should probably take a glance at the high level portions of this given you had a lot of experience Razor's config handling
| ParseClientConfiguration(settings, scope.Section); | ||
| } | ||
|
|
||
| OnReload(); |
There was a problem hiding this comment.
Do we want to wait to trigger a reload until all scopes are updated?
There was a problem hiding this comment.
That's an interesting observation... they will likely get double updated (once from the OnReload() and once from the update call. It should probably move to the bottom just for consistency.
| return Unit.Value; | ||
| } | ||
|
|
||
| ParseClientConfiguration(request.Settings); |
There was a problem hiding this comment.
ParseClientConfiguration expects a JSON object but there's no promise that the request.Settings is JSON is there? This is the issue we were having on the Razor front when approaching configuration. We've had to build our LanguageServer to somewhat special case each client because the configurations in each client can have:
- Different configuration identifiers
- Possibly different configuration formats (XML)
Instead of baking the configuration parser/expectation of JSON into the base class it'd probably be best to split that out into a service that a language server can replace.
There was a problem hiding this comment.
Do you have examples of a client that gives back xml as configuration (just my knowledge/testing)? I'll look at adding the service this weekend.
There was a problem hiding this comment.
So by default VS stores its configurations in XML format however the LSP client for VS doesn't currently support interacting with those XML based configurations. Therefore, for the time being they brought over JSON configurations to act as a replacement of the XML based configurations but all of the new JSON configs do not translate to XML and do not auto-map to the XML configs either. Therefore VS is trying to figure out how it wants its configuration story to roll out so it can work with new language server.
tl;dr; I don't know of any currently; however, it's definitely possible it'll come down in the future.
| @@ -61,6 +61,10 @@ class HandlerDescriptor : ILspHandlerDescriptor, IDisposable, IEquatable<Handler | |||
| typeof(DelegatingRequest<>).IsAssignableFrom(@params.GetGenericTypeDefinition()) || | |||
| typeof(DelegatingNotification<>).IsAssignableFrom(@params.GetGenericTypeDefinition()) | |||
| ); | |||
| if (handler is IOnStarted started) | |||
| { | |||
| StartedDelegate = started.OnStarted; | |||
| /// </summary> | ||
| /// <param name="scopeUri"></param> | ||
| /// <returns></returns> | ||
| Task<IDisposableConfiguration> GetScopedConfiguration(Uri scopeUri); |
There was a problem hiding this comment.
Hmm, is there value in exposing that this is an IDisposableConfiguration here? Is the LanguageServer author expected to call dispose on it?
There was a problem hiding this comment.
Yeah, the high level idea here is that a language server wants to get configuration information for a specific document (at least from the vscode side anyway). This would allow them to see any overrides that might have happened for example vscode lets you do "[csharp]":{} to override settings for a specific language.
Since there is nothing in the protocol today that lets us ask for settings for a generic language or file type, we instead have to go this route. The reason it's disposable is because we don't want to have to double information for files that are no longer in scope, as each refresh will update all the active scopes.
There was a problem hiding this comment.
I understand that aspect of it but by exposing the IDisposeableness of the scoped configuration I as the Razor langauge server author could call dispose which wouldn't be expected I'd imagine. I'd think you (as O# lib) would want to reserve the right to dispose
There was a problem hiding this comment.
The problem is on the LSP side I don't know when you're "done" with that configuration, hence the disposable aspect of it.
It could be in context of a file or it could be in context of a file that is currently being acted upon. In essence you wanted the configuration for that file, and you take ownership of when you're done. On the library side we're only going to update the configuration for you automagically.
I could be something that is unnecessary and maybe GetConfiguration is enough.
Codecov Report
@@ Coverage Diff @@
## master #207 +/- ##
==========================================
- Coverage 48.29% 43.92% -4.38%
==========================================
Files 350 356 +6
Lines 7669 7814 +145
Branches 957 975 +18
==========================================
- Hits 3704 3432 -272
- Misses 3665 4092 +427
+ Partials 300 290 -10
Continue to review full report at Codecov.
|
This is a wip that will need some more unit tests.
In essence the goal here is to enable support for both
workspace/didChangeConfigurationandworkspace/configurationthrough the MECIConfigurationinterface. such that as configuration changes happen on the client, they are reflected correctly in the configuration of the server. This opens the door to usingIOptionsMonitorin places as well.Another part is to allow support for the
scopeUriportion ofworkspace/configurationreturning a disposable interface that is alsoIConfigurationand managing the state for that object as long it as it hasn't been dispoed. For example keeping the configuration of all the currently opened files is probably something that will be needed.And finally allowing configuration to just be queried and inspected for different areas of the client. Basically just to make the work of getting that information a little easier to consume.