flux-lsp
flux-lsp copied to clipboard
Variable completion casts too wide of a net
In the server tests, we have some code that looks like this:
import "strings"
import "csv"
cal = 10
env = "prod01-us-west-2"
cool = (a) => a + 1
c
The test then checks to see what would be expanded from c
. This is the current list it would expand to:
"buckets",
"cardinality",
"chandeMomentumOscillator",
"columns",
"contains",
"contrib/RohanSreerama5/naiveBayesClassifier",
"contrib/anaisdg/anomalydetection",
"contrib/bonitoo-io/servicenow",
"contrib/bonitoo-io/tickscript",
"contrib/bonitoo-io/victorops",
"contrib/chobbs/discord",
"count",
"cov",
"covariance",
"csv",
"cumulativeSum",
"dict",
"difference",
"distinct",
"duplicate",
"experimental/csv",
"experimental/record",
"findColumn",
"findRecord",
"getColumn",
"getRecord",
"highestCurrent",
"hourSelection",
"increase",
"influxdata/influxdb/schema",
"influxdata/influxdb/secrets",
"internal/location",
"logarithmicBins",
"lowestCurrent",
"reduce",
"slack",
"socket",
"stateCount",
"stateTracking",
"testing/expect",
"truncateTimeColumn",
I haven't looked at the code, but it looks like it's expanding to anything that could possibly have a c
(or even a C
) in it. If I type c
I expect things that start with c
(and not C
) to be possibly completions.
Providing completion for anything that contains the identifier seems good to me, though I think it should prefer things that start with the identifier that we are completing (and thereby list those first).
It is possible to use https://docs.rs/lsp-types/0.93.0/lsp_types/struct.CompletionItem.html#structfield.sort_text to better control the order but I think VS code (and other clients) actually do fuzzy matching/sorting on what the server sends to give better UX https://github.com/microsoft/language-server-protocol/issues/898 so trying to override that may make things worse.
Providing completion for anything that contains the identifier seems good to me
I'm not sure I agree, at least in the case of short identifiers. Typing c
and getting buckets
as a suggestion seems incorrect. The Monaco client seems to agree. It looks like it drops anything that doesn't start with c
or have a camel case C
in it.
![image](https://user-images.githubusercontent.com/60306/166862532-7651c330-8faa-4783-88a4-bf885b4e4164.png)
The client seems to be hiding this issue, but I think we should suggest less as well.
I sort of imagine it'd be nice for clients to get all the options and be able to apply whatever weighting they see fit.
Otherwise, I think we could calculate the levenshtein distance ourselves and send only the N strongest scores. For a single character search, things starting with that character should score better. The more characters entered, the better the results should be.
The client seems to be hiding this issue, but I think we should suggest less as well.
I don't see it as hiding it, it just allows the client to decide how to display the completions in the UI. For that example it seems to already be doing what we want (?) so us sending "extra" completions doesn't seem to cause an issue.
![image](https://user-images.githubusercontent.com/60306/170508916-448766bd-8b87-4a21-8d2d-58c054ab3fa6.png)
Here's an example that occurs because the client is trying to be "too smart" with what it considers a complete list. It's re-using a previous list of completables in a way that is more misleading than anything. After seeing this interaction, I'm not sure clients can be trusted to complete things in the way that makes sense, so I'm in favor of never saying that the completion items are complete.
Strange, why is it reusing the completions in this case? I'd expect that typing .
would trigger a new completion request automatically, and the response then would only return properties from the schema
object.