-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Hook up DidChangeConfiguration from client #66861
Conversation
…ne/roslyn into dev/shech/LSPConfiguration
eb3fdea
to
3a2a663
Compare
internal partial class DidChangeConfigurationNotificationHandler | ||
{ | ||
// TODO: all the supported options here | ||
private static readonly ImmutableArray<ISingleValuedOption> s_supportedGlobalOptions = ImmutableArray.Create<ISingleValuedOption>(); |
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.
Why do we need "supported" options? Why not just allow any 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.
Do you mean here we don't care if the options is meaningful to the cilent not and just asking it for all the options?
If case the client doesn't have that, (it might return null), we just ignore the value and fall back to our default value?
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.
pretty much
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Options/Option2.cs
Outdated
Show resolved
Hide resolved
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.
Mostly all seems reasonable - some minor feedback.
src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs
Outdated
Show resolved
Hide resolved
...s/LanguageServer/Protocol/Handler/Configuration/DidChangeConfigurationNotificationHandler.cs
Outdated
Show resolved
Hide resolved
...s/LanguageServer/Protocol/Handler/Configuration/DidChangeConfigurationNotificationHandler.cs
Outdated
Show resolved
Hide resolved
...s/LanguageServer/Protocol/Handler/Configuration/DidChangeConfigurationNotificationHandler.cs
Outdated
Show resolved
Hide resolved
...s/LanguageServer/Protocol/Handler/Configuration/DidChangeConfigurationNotificationHandler.cs
Outdated
Show resolved
Hide resolved
...s/LanguageServer/Protocol/Handler/Configuration/DidChangeConfigurationNotificationHandler.cs
Outdated
Show resolved
Hide resolved
...guageServer/ProtocolUnitTests/Configuration/DidChangeConfigurationNotificationHandlerTest.cs
Outdated
Show resolved
Hide resolved
...guageServer/ProtocolUnitTests/Configuration/DidChangeConfigurationNotificationHandlerTest.cs
Outdated
Show resolved
Hide resolved
...guageServer/ProtocolUnitTests/Configuration/DidChangeConfigurationNotificationHandlerTest.cs
Outdated
Show resolved
Hide resolved
...guageServer/ProtocolUnitTests/Configuration/DidChangeConfigurationNotificationHandlerTest.cs
Show resolved
Hide resolved
...guageServer/ProtocolUnitTests/Configuration/DidChangeConfigurationNotificationHandlerTest.cs
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
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.
I'm good with this
Tomas is OOF now, and I have synced with him before he left, as long as David is fine with the change he is also fine with that
Linked the server to client to use the DidChangeConfiguration and workspace/configuration to update the configuration value.
Some other change:
The model used is the LSP pull model from the server, it is done in this way.
The section sends to the client is in this pattern:
{optionGroupName}.{optionName}
{csharp|visual_basic}.{optionGroupName}.{optionName}
It is done in this way because I think Roslyn server is CSharpVisualBasicLspServer, so the server should ask all the options for both languages. It is the client's responsibility to figure out which language should be supported. If the client only support C# or VB, it should give empty value to server for the supported language options.