feat: allow configuration through workspace/didChangeConfiguration #316
Conversation
|
This is literally my first time writing Rust, ever. lmao. Furthermore, I started this in draft because I'm not sure what you want with regards to testing and such. Is it safe to change workspace settings at runtime? No idea. Is my code nasty? Certainly. Anyways, let me know how/if you'd like to proceed with this. I thought it would be pretty straightforward after thinking it through while reading #302 and, I don't know, kinda felt the random urge to finally give Rust a try :) |
There was a problem hiding this comment.
Hey hey! 👋
First of all, thank you for the contribution :) And sorry for the late reply.
The approach looks good to me; I first thought that it would be nicer to use a update_settings method for this, but I think it's fine as it is.
I have a few remarks regarding code, but great first Rust PR 👍
| @@ -386,11 +386,11 @@ impl Session { | |||
| /// This function attempts to read the `postgrestools.jsonc` configuration file from | |||
| /// the root URI and update the workspace settings accordingly | |||
| #[tracing::instrument(level = "trace", skip(self))] | |||
| pub(crate) async fn load_workspace_settings(&self) { | |||
| pub(crate) async fn load_workspace_settings(&self, params: Option<PartialConfiguration>) { | |||
There was a problem hiding this comment.
I liked the extra_config name – would you mind renaming this as well?
| configuration: match extra_config { | ||
| Some(config) => fs_configuration.clone().merge(config), | ||
| None => fs_configuration, | ||
| }, |
There was a problem hiding this comment.
I don't think we need the extra allocation (caused by clone()) – plus, if the extra_config has any gitignore related settings, these would be ignored.
Maybe it is better to do a fs_configuration.merge_with(extra_config) a few lines above (if extra_config is Some(..))?
If you use the .merge_with method from the biome Merge trait, you probably don't even need an extra method 👍
Let me know if you need help with that.
| pub fn merge(&mut self, other: Self) -> Self { | ||
| self.merge_with(other); | ||
| self.clone() | ||
| } |
There was a problem hiding this comment.
If you do what I wrote above, we don't need this helper and the clone 👍🏻
What kind of change does this PR introduce?
It enables passing workspace settings via the workspace/didChangeConfiguration notification. This in turn enables the client to specify settings dynamically, rather than being limited to configuration files. This is a solution to #302. See example usage (with lspconfig) here: willruggiano/neovim.drv@9aa06ad.
What is the current behavior?
There is none. The payload of this handler is currently ignored.
What is the new behavior?
The configuration received by the handler is merged with the fs configuration.