-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Added option to provide a custom config file to the lsp. #460
Added option to provide a custom config file to the lsp. #460
Conversation
helix-lsp/src/lib.rs
Outdated
Err(e) => { | ||
log::info!( | ||
"Loading lsp config file for {} failed => {}", | ||
language_config.language_id, | ||
e | ||
); | ||
None | ||
} |
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 don't think we want to show that it failed when the custom config isn't there.
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.
Yeah agreed
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 didn't know it is this easy. Looks good to me other than comment I left. Tested but not sure if it works, probably it works.
But should we make use of languages.toml
to do the custom configuration instead? kak-lsp seemed to do it with set-option -add global lsp_server_configuration rust.clippy_preference="on"
but I wonder what are the limitations.
Maybe @ul have a better idea. @ul sorry for pinging you even but how was the choice to have custom configuration rather than doing the original json like object in kak-lsp?
helix-lsp/src/lib.rs
Outdated
@@ -410,6 +426,17 @@ impl LspProgressMap { | |||
} | |||
} | |||
|
|||
fn load_lsp_config_file(language: &str) -> anyhow::Result<Option<serde_json::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.
Instead of extra files, what do you think about making this a config
key under the language_server toml key? Then one can use
config = """
{
...
}
"""
to declare the config
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.
That was my first thought, (my second was to put it in the helix config file), but I decided against that because editing json in a toml file wasn't very ergonomic. That said, I am not against it; I will make that chance.
You can easily test it by starting helix with |
The idea was to make it easier to dynamically define these values inside Kakoune config without involving an external JSON processor (as Kakoune doesn't have a built-in JSON handling API available in configs), at least in simple cases. How wise was it and how widely is it leveraged in the wild? I don't know, I personally never used it. Maybe having it as original JSON and relying on piping through |
helix-core/src/syntax.rs
Outdated
@@ -35,6 +35,7 @@ pub struct LanguageConfiguration { | |||
pub scope: String, // source.rust | |||
pub file_types: Vec<String>, // filename ends_with? <Gemfile, rb, etc> | |||
pub roots: Vec<String>, // these indicate project roots <.git, Cargo.toml> | |||
pub custom_config: Option<String>, |
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.
Nit: let's just call it config
.
1f9f273
to
bbe6b43
Compare
This allows for a runtime/lsp_configs/.json file to be provided. When detected, it will be send to the lsp, allowing to user to configure the lsp.
Feedback is welcome, especially regarding the place of the file read; the naming of the directory and if this is the correct approach.
I added a sample file to facilitate discoverability.