feat: configure Workspace Symbols to return all symbols on empty query #293
Conversation
doorgan
force-pushed
the
doorgan/configuration
branch
3 times, most recently
from
982fef7 to
34c17d9
Compare
doorgan
force-pushed
the
doorgan/configuration
branch
from
34c17d9 to
9a679fc
Compare
| |> maybe_add_watched_extensions(settings) | ||
| new_config = | ||
| old_config | ||
| |> set_dialyzer_enabled(settings) |
There was a problem hiding this comment.
I'm confused, do we support dialzyer?
There was a problem hiding this comment.
The option was already there and I didn't remove it. Looking into the codebase we're not doing anything with it so I'll remove it altogether.
Looking at discussions from when this was still Lexical, there were some attempts to add dialyzer and there was a TODO for it: lexical-lsp/lexical#95
But it never got implemented, and this config option seems to be a leftover from that time
|
|
||
| | Setting | Type | Default | Description | | ||
| |---------|------|---------|-------------| | ||
| | `workspaceSymbols.minQueryLength` | integer | `2` | Minimum characters required before workspace symbol search returns results. Set to `0` to return all symbols with an empty query. | |
There was a problem hiding this comment.
Do these need to be camelCased? I feel like some servers have them camel cased even if their language is not cameCased, possibly because the VSCode settings use that convention.
There was a problem hiding this comment.
They don't need to be, I was following convention as you mention. I have no opinion on which casing to use, we can use snake_case instead
There was a problem hiding this comment.
Yeah, I can't tell if I want it to be snake cased cuz I think it matters or if my brain just sees camelCase and is like "must destroy"
I'll lean toward doesn't matter who cares.
But we'll want this documented on the website after we merge.
There was a problem hiding this comment.
Yep, I'll update expert-lsp.org after this
| defmodule Expert.Configuration.WorkspaceSymbols do | ||
| @moduledoc false | ||
|
|
||
| defstruct min_query_length: 2 |
There was a problem hiding this comment.
out of curiosity - what's the reason behind having this default to 2 and allowing user to configure this altogether? performance? because to me it looks like 0 as hardcoded would be a reasonable thing to have and I don't recall other LSes having such config
There was a problem hiding this comment.
It's the previous behavior, I had to type 2 characters to get completions before this change. Hardcoding 0 makes us return every result and let the client filter them, which is the NextLS behavior. Having >0 causes the server to filter results first, which is the Lexical behavior.
There was a problem hiding this comment.
Yeah I get this. I was wondering why 2 was in the first place here 😅
There was a problem hiding this comment.
Not sure! Maybe because a single letter wasn't considered specific enough for filtering to be meaningful?
There was a problem hiding this comment.
I think this might be editor-specific. VSCode and IntelliJ I know have even some ML models for sorting such things. otoh probably helix/neovim (not using them) just throw stuff as they get from the LSP. So I guess, perhaps the original author picked what worked in his editor? 🤔
Closes #140
Closes #155
There's a rather big change to the way we do configuration, not only to support this new option, but also becase there was a race condition that prevented config changes from being applied reliably, as requests other than didChangeConfiguration could be handled at the same time as eg textDocument/didOpen, which would return a state with the old config, overwriting the new one.
The new approach is to use
:persistent_terminstead. I opted for this rather than a genserver or an agent because configuration changes are infrequent, and removing the config from the server state is enough to eliminate that race condition