Passing extra formatting options to LSPs #2635
Conversation
- adds optional field 'format' to [[language]] sections in 'languages.toml' - passes specified options the LSPs via FormattingOptions
book/src/languages.md
Outdated
| name = "typescript" | ||
| auto-format = true | ||
| # pass format options according to https://github.com/typescript-language-server/typescript-language-server#workspacedidchangeconfiguration omitting the "[language].format." prefix. | ||
| format = { "semicolons" = "insert", "insertSpaceBeforeFunctionParenthesis" = true } |
There was a problem hiding this comment.
This was a mistake we did with language server configuration: I'd prefer format was nested under the language-server key. Though maybe we can keep it toplevel
There was a problem hiding this comment.
It would make sense inside the language server configuration, especially if we ever plan to support multiple LS per language. I have seen neovim setups where multiple LS get attached to a buffer, mostly for chaining formatting functionality. Can't say that i liked it. Have you guys thought in that direction? I haven't noticed the topic here, but i am just getting into the project.
There was a problem hiding this comment.
i guess we need this. 6c29b0e nests it inside the lsp config now.
helix-view/src/document.rs
Outdated
| let prop = if let Some(s) = value.as_str() { | ||
| lsp::FormattingProperty::String(s.to_owned()) | ||
| } else if let Some(b) = value.as_bool() { | ||
| lsp::FormattingProperty::Bool(b) | ||
| } else if let Some(n) = value.as_i64() { | ||
| lsp::FormattingProperty::Number(n as i32) | ||
| } else { | ||
| log::warn!("invalid formatting property type {:?} for {}", value, key); | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
I think this should work:
let prop: lsp::FormattingProperty = serde_json::from_value(value)There was a problem hiding this comment.
great. i changed it to 563fccf
for (key, value) in fmt.iter() {
if let Ok(prop) = serde_json::from_value(value.clone()) {
properties.insert(key.to_owned(), prop);
}
}
helix-core/src/syntax.rs
Outdated
| @@ -78,6 +78,9 @@ pub struct LanguageConfiguration { | |||
|
|
|||
| #[serde(default)] | |||
| pub auto_format: bool, | |||
| #[serde(default, skip_serializing, deserialize_with = "deserialize_lsp_config")] | |||
| pub format: Option<serde_json::Value>, | |||
There was a problem hiding this comment.
Actually, why not just use HashMap<String, FormattingProperty> here?
There was a problem hiding this comment.
That would make helix-core dependent on the lsp crate, just to get the FormattingProperty type into LanguageConfiguration.
no longer a concern with 6c29b0e
helix-lsp/src/client.rs
Outdated
| for (key, value) in format { | ||
| // upstream properties take precedence | ||
| if merged_options.properties.get(key).is_some() { | ||
| continue; | ||
| } | ||
| if let Ok(prop) = serde_json::from_value(value.clone()) { | ||
| merged_options.properties.insert(key.to_owned(), prop); | ||
| } | ||
| } |
There was a problem hiding this comment.
| for (key, value) in format { | |
| // upstream properties take precedence | |
| if merged_options.properties.get(key).is_some() { | |
| continue; | |
| } | |
| if let Ok(prop) = serde_json::from_value(value.clone()) { | |
| merged_options.properties.insert(key.to_owned(), prop); | |
| } | |
| } | |
| let format_config: HashMap<String, lsp::FormattingProperty> = serde_json::from_value(format.clone()); | |
| // options take precedence over format_config | |
| // extend replaces values with existing keys with new values returned from the iterator. | |
| options = format_config.extend(options); |
There was a problem hiding this comment.
thanks. cleaned this up and eliminated clone
adds optional field format to [[language]] sections in languages.toml
passes specified options to the LSPs via FormattingOptions
updated documentation accordingly
typescript example: