Simplify LSP settings #865
Open
+85
−264
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DavisVaughan
requested changes
Jul 16, 2025
Comment on lines
+106
to
+108
| use crate::lsp::config::SETTINGS; | ||
|
|
||
| for setting in SETTINGS { |
There was a problem hiding this comment.
Suggested change
| use crate::lsp::config::SETTINGS; | |
| for setting in SETTINGS { | |
| for setting in crate::lsp::config::SETTINGS { |
That feels very AI 😆
| @@ -284,121 +275,35 @@ pub(crate) fn did_change_formatting_options( | |||
| // `insert_final_newline` | |||
| } | |||
|
|
|||
| use crate::lsp::config::SETTINGS; | |||
There was a problem hiding this comment.
oh god please don't put a use declaration here
if this is ai please update the ai instructions md file
Comment on lines
-314
to
-324
| // For document configs we collect all pairs of URIs and config keys of | ||
| // interest in a flat vector | ||
| let document_keys = VscDocumentConfig::FIELD_NAMES_AS_ARRAY; | ||
| let mut document_items: Vec<ConfigurationItem> = | ||
| itertools::iproduct!(uris.iter(), document_keys.iter()) | ||
| .map(|(uri, key)| ConfigurationItem { | ||
| scope_uri: Some(uri.clone()), | ||
| section: Some(VscDocumentConfig::section_from_key(key).into()), | ||
| }) | ||
| .collect(); | ||
| items.append(&mut document_items); |
There was a problem hiding this comment.
Hmm isn't this important? I'm pretty sure we collected document config on a per open document basis and now we just collect it once with a scope_uri: None which doesn't seem right
Comment on lines
-366
to
-368
| if changed { | ||
| lsp::spawn_diagnostics_refresh_all(state.clone()); | ||
| } |
There was a problem hiding this comment.
We dropped a refresh here
| let config: DocumentConfig = config.into(); | ||
|
|
||
| // Finally, update the document's config | ||
| state.get_document_mut(&uri)?.config = config; |
There was a problem hiding this comment.
Yea see we dropped the per document config update
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I think you'll like this one @DavisVaughan.
Branched from #859. I was concerned at how hard it was to add a new setting and section. I think the refactor in this PR will make things much simpler and easier:
Add a generic
Settingtype that allows us to flatten a representation of all our settings in a flat arraySETTINGSthat is easy to loop over. This drastically simplify the updating logic where we send all keys of interest to the client in a flat array, as we no longer need any bookkeeping.The setting objects in this array contain the conversion-from-json logic as simple methods assigned to a closure field. The default handling is still delegated to
Defaultmethods in our setting types.Remove the split between VS Code and LSP settings. If we want to add vscode agnostic settings in the future, we could add editor aliases in the
SETTINGSarray..