-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
Load configuration during initialize handling #84
Conversation
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.
initialization_options
is for arbitrary user data. This loads the whole value and parse it as configuration. Is this a common practice? How do you pass initial configuration through this with LC-neovim?
Not sure if we should put configurations under another layer of indirection, like initialization_options.settings
, for future-compatibility. Would it complicate the setup?
crates/nil/src/server.rs
Outdated
@@ -163,7 +163,20 @@ impl Server { | |||
None => std::env::current_dir().expect("Failed to the current directory"), | |||
}; | |||
|
|||
*Arc::get_mut(&mut self.config).expect("No concurrent access yet") = Config::new(root_path); | |||
*Arc::get_mut(&mut self.config).expect("No concurrent access yet") = { |
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.
Please unwrap the block and move the Arc setting line below.
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.
done
Allows to configure the server when client doesn't support `workspace/configuration` method, for example, LanguageClient-neovim.
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.
initialization_options
is for arbitrary user data. This loads the whole value and parse it as configuration. Is this a common practice? How do you pass initial configuration through this with LC-neovim?
LC-neovim allows to specify these options in global or workspace-specific config: https://github.com/autozimu/LanguageClient-neovim/blob/next/doc/LanguageClient.txt#L99-L112 and https://github.com/autozimu/LanguageClient-neovim/blob/next/doc/LanguageClient.txt#L289-L301. It seems it was intended to have workspace/configuration
support, but that didn't happen yet.
Not sure if we should put configurations under another layer of indirection, like
initialization_options.settings
, for future-compatibility. Would it complicate the setup?
Notice that examples in LanguageClient-neovim docs mention that these are the values that should be expected by workspace/configuration
response. I assume that other language servers use same structure for both. For example, gopls
uses the same function in initialize
and workflow/configuration
: https://github.com/golang/tools/blob/master/gopls/internal/lsp/general.go#L60 and https://github.com/golang/tools/blob/master/gopls/internal/lsp/general.go#L517. rust-analyzer
loads config directly from that field as well: https://github.com/rust-lang/rust-analyzer/blob/master/crates/rust-analyzer/src/bin/main.rs#L189-L190
crates/nil/src/server.rs
Outdated
@@ -163,7 +163,20 @@ impl Server { | |||
None => std::env::current_dir().expect("Failed to the current directory"), | |||
}; | |||
|
|||
*Arc::get_mut(&mut self.config).expect("No concurrent access yet") = Config::new(root_path); | |||
*Arc::get_mut(&mut self.config).expect("No concurrent access yet") = { |
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.
done
04d67df
to
c4eee44
Compare
Thanks! |
Allows to configure the server when client doesn't support
workspace/configuration
method, for example, LanguageClient-neovim.