feat: external completions for commands/flags

[herlon214]

Description

This PR adds support to use external completions (like carapace) instead of Nushell completions by adding a new config called external_completer which is optional.

let carapace_completer = {|spans| 
    carapace $spans.0 nushell $spans | from json
}

# The default config record. This is where much of your global configuration is setup.
let-env config = {
  external_completer: $carapace_completer

Basically, if you set one external_completer it will use it instead of our current Nushell completion flow for commands and flags.

Current issues:

• Flags aren't being handled (e.g: git config --<tab>)
• Resolve alias before sending to external completer
• Remove extra a added in the spans
• Open quotes aren't being handled (e.g: chown "<TAB>) <--- We will leave this one up to the external_completer Nu function handle

Tests

Make sure you've done the following:

• Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
• Try to think about corner cases and various ways how your changes could break. Cover them with tests.
• If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

Make sure you've run and fixed any issues with these commands:

• cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
• cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
• cargo test --workspace --features=extra to check that all the tests pass

[herlon214]

What should we do if we fail to eval the block?

[elferherrera]

I'm of the idea that it should return a Result, but that will change more things in the code

[herlon214]

Yeah, but then it goes back to the fn completion_helper(...) -> Vec<Suggestion> { which should handle it anyway and return a Vec<Suggestion>

[herlon214]

Would also be nice to move the $external_completer to outside config.nu, somehow.

[elferherrera]

I'm of the idea that it should return a Result, but that will change more things in the code

[kubouch]

I found these issues testing this PR:

1. ls -<tab> will trigger the external completer.
2. Just <tab> on an empty line, or l<tab> also triggers the external completion.

In both cases, the external completer should be triggered after trying Nushell's completions, not before, because it prevents us from completing built-in commands and flags correctly.

But in principle, these two cases seems like the right places where to trigger the external completer. It just needs to be moved lower in priority.

[herlon214]

@kubouch what do you mean by moving them lower in priority?
If we trigger Nushell ones it automatically returns that result and will never reach the external one.

We need to check if nushell internals return empty, if so we trigger the external, is that what you mean?

[kubouch]

@kubouch what do you mean by moving them lower in priority? If we trigger Nushell ones it automatically returns that result and will never reach the external one.

We need to check if nushell internals return empty, if so we trigger the external, is that what you mean?

Yeah, that's what I mean. Check if Nushell completions return something and if not, try the external completer.

[herlon214]

@kubouch what do you mean by moving them lower in priority? If we trigger Nushell ones it automatically returns that result and will never reach the external one.
We need to check if nushell internals return empty, if so we trigger the external, is that what you mean?

Yeah, that's what I mean. Check if Nushell completions return something and if not, try the external completer.

Got it, I will update the PR then

[herlon214]

I found one issue with the current code, once we don't have completions for gh alias nu should trigger the external compelter. But instead it's returning a list of all commands:

[rsteube]

There also seems to be a fundamental step missing compared to how nushell passes arguments to a command.
I think the slicing is correct but there should be some way to evalute the slices.

Here 'good first issue', is passed with the quotes:

gh issue list --label 'good first issue',<TAB>
# ''good first issue',0.67'                     Issues that should be fixed in v0.67
# ''good first issue',Stale'                    used for marking issues and prs as stale

More apparent with the string interpolation which results in slices kinda like this:

ls $"($env.HOME)"<TAB>
# [ 'ls', '$"', '(', '$env', 'HOME', ')', '"a' ]

[kubouch]

@rsteube The eval step for the argument is a good idea, but I would leave it for a future work. I'd rather get this PR merged first for simple cases.

About the quotes, 'good first issue', is one string from Nushell's point of view. It doesn't know , is supposed to split the arguments, that's just an interpretation of the external command that we can't assume. We might want to expand the external command parsing to be smarter, but we'd have to change Nushell parsing rules which is also outside the scope of this PR. Ideally, there should be no parsing involved in the completions code, we should just reuse existing logic, otherwise, we'd create a mess.

[kubouch]

I found one issue with the current code, once we don't have completions for gh alias nu should trigger the external compelter. But instead it's returning a list of all commands:

I think the error here might be related to the long-standing bug that if there is an empty input, it defaults to completing all the commands. Like when you press <tab> on an empty line, you get a screen full of commands which is not very useful. Also, completing external commands when you already typed some words (like gh alias) is almost always not what you want.

You want to complete commands only in two cases:

1. After you typed more than 0 non-space letters
2. All the words before the cursor position match a known command or subcommand (like completing path exp<tab>, you should get path expand suggestion). If there is no match for a known command / subcommand, then no command completion should be given and external completion would kick in.

Also, it seems to me, the file completion should have a priority over command completion. Currently, it first tries commands, then files but I'm wondering if it should be the other way.

So, to sum up, it seems the error here is in the completion logic itself, not in introducing the external completer. What we could do is to land this PR and make a new PR to fix the other completion issues. However, we should probably make some list to document the cases where we know the completions are broken.

There is also #6257 which might be about fixing the same command completion issue. It introduces a new syntax shape, though, which we thought might not be the best approach. You might want to take a look at it and maybe work together with @merelymyself if you want to address the wrong command completions.

[herlon214]

How can we have a minimum working version so the users can test and give some feedback. Maybe we can address some corner cases later given user's feedback?
So we can land this PR and maybe open issues for the known things.

[kubouch]

Yes, the gh alias issue is already present without your modification, so we can leave it for later. There is still some failing test in this PR, though.

[herlon214]

Yeah @kubouch I think that test failing is related to the issue we discussed. Should I remove that test then?

I can give priority to external completer for commands for now, then once we fix the issue discussed before we give priority to Nu again, what do you think?

[kubouch]

OK, if it's related to the gh alias issue, then let's #[ignore] the test for now. Could you please create an issue that documents the problem.

I think if you prioritize the external completer, it would call to it before completing the Nushell command which is not what we want. I suspect prioritizing the external completer would need a bit of extra logic I outlined earlier to make it work reliably. Like path exp<tab> should auto-complete the Nushell command first and not call external completer at all.

And one more thing: Because you added a new config option, could you please put an example entry to the default config (commented out or set to $nothing by default)? It might be worth adding to the book somewhere as well once it gets merged.

[kubouch]

OK, let's give this a try! Thanks for the effort!

[rsteube]

@herlon214 The merged version doesn't work for me (main branch). Did something change?

[herlon214]

@rsteube I think the priority of the completions changed, and if I'm not mistaken currently it isn't working well due to one issue being fixed on #6257