nushell
Allow setting span in custom/extern completers
Description
Closes https://github.com/nushell/nushell/issues/15863. This PR allows custom and extern completers to set the span field for the Suggestions they provide.
The motivation for this is mainly making it easier to make completers for Zoxide.
User-Facing Changes
Users will be able to choose which part of the buffer to replace. This is useful if you also want to replace one of the previous arguments to a command, or if you only want to replace part of an expression.
Tests + Formatting
Didn't add any tests, but did manually try the following:
> def comp [context, pos] { [{value: mwahahaha, description: bar, span: {start: 0, end: $pos}}] }
> def foo [arg: string@comp] {}
After creating those definitions, foo <TAB> will replace the command line with mwahahaha.
After Submitting
Needs to be mentioned in the completer docs.
We should only support this if everything that sees those spans is tolerant to bad indices. We don't want reedline to panic when you index out of bounds or between char boundaries of multibyte-utf8
Good point, Reedline definitely isn't bad-span-proof yet. Marking this as draft for now.
We should only support this if everything that sees those spans is tolerant to bad indices. We don't want reedline to panic when you index out of bounds or between char boundaries of multibyte-utf8
@sholderbach https://github.com/nushell/reedline/pull/915 should protect against both of those now.
Didn't add any tests, but did manually try the following:
Should be easy to test by measuring SemanticSuggestion.suggestion.span?
would like to see a test or two to protect this functionality from breaking in the future
Sorry about that, just pushed tests for both custom and external completers
Okay resolved merge conflicts and fixed up tests, should be good to go now
@ysthakur, I get confused when actually trying to use this feature.
How should I set the span start properly? I mean in a complicated input like ls | foo <tab>.
Nevermind, I figured it out: $pos - ($context | str length). But it's probably easier if we allow overriding only the start/end half of the span
Yeah, it has this really weird behaviour.
commandlinein inaccessable in the completion function$contextonly provides args of the current function of the pipeline untill the cursor$posprovides cursor position relative to thecommandline(which is inaccessable)- span overrides whole
commandlineinstead of just current function
I will open an issue about that shortly
Yeah, it has this really weird behaviour.
commandlinein inaccessable in the completion function$contextonly provides args of the current function of the pipeline untill the cursor$posprovides cursor position relative to thecommandline(which is inaccessable)- span overrides whole
commandlineinstead of just current functionI will open an issue about that shortly
You want $pos to be relative to current command start? Will it be a breaking change?
You want $pos to be relative to current command start? Will it be a breaking change?
Yes, but it doesn't matter anyways. Because before landing this pr, I don't think there were any valid usecase of $pos with the current behaviour.
But what would actually be breaking is the change of $context.
What I thought (and want) about the args of the completion commands were:
$context: all args of a single command in a pipeline including the command name itself$pos: position of the cursor relative to the start of the$context
So essentially,
$old_context == ($new_context | str substring 0..<$new_pos)
You want $pos to be relative to current command start? Will it be a breaking change?
Yes, but it doesn't matter anyways. Because before landing this pr, I don't think there were any valid usecase of
$poswith the current behaviour.
I get your point: there're 2 referencing pointers instead of just $pos. But it's still painful if you want to replace from the middle of current argument list. I'm not sure if such minor benefit worth a breaking change.
But what would actually be breaking is the change of
$context.What I thought (and want) about the args of the completion commands were:
$context: all args of a single command in a pipeline including the command name itself
$pos: position of the cursor relative to the start of the$contextSo essentially,
$old_context == ($new_context | str substring 0..<$new_pos)
It's not possible to get arguments after the cursor for now. Input text gets truncated before passing into the completion system. There's a long existing TODO note for changing this behavior, you can try implementing it if you get the passion. But it's another breaking change we need to discuss before coding.
Hmm, I see.
There's a long existing TODO note for changing this behavior
Can you mention the issue
Hmm, I see.
There's a long existing TODO note for changing this behavior
Can you mention the issue
https://github.com/nushell/nushell/blob/c8549448452d28d1a2fbf17ecc63b331c303374c/crates/nu-cli/src/completions/completer.rs#L191
BTW, I don't think that completion depending on argument values after the cursor is a good practice.
Maybe you need to double check your intention.
BTW, I don't think that completion depending on argument values after the cursor is a good practice.
If we decide to continue having our LSP use the same completer that the CLI uses, then we might need it in the future. Suppose someone needs to pass a value of type record<a: int, b: int> to a function. They type {, b: int}, then move their cursor back: {|, b: int} (the | is the cursor). Now we can provide a: or b: as suggestions based on the expected type of the value. But if we look beyond the cursor, we can see that the key b is already provided, so the only sensible suggestion is a:.
That said, for advanced cases like this, I think the LSP should have its own logic, and the nu-cli completion logic can remain "dumb."
That said, for advanced cases like this, I think the LSP should have its own logic, and the nu-cli completion logic can remain "dumb."
That's what it is now. LSP completion doesn't truncate the input, doesn't insert the placeholder unless the cursor is at some sensitive places.