Fish dynamic completion

[ModProg]

Document in the PR description why this couldn't follow the bash model for completions

I didn't follow the bash model for completions because I didn't fully understand how it worked, and wanted to have the CompletionContext as a way for the shell specific part to handle how it selects the completions.

I thought it should be possible to use the methods I added in mod to use for bash, but I don't know for sure, as I didn't understand what all the options there did.

Document in the PR description how these completions work

End developer api

The completion logic is made available in too ways:

1. Use the Completable trait, it supports both methods to just trigger completions and a direct replacement for Opts::parse() that will do completions or parse.
2. Using the CompleteComand::augment_subcommands and manually handle the outcome by calling CompleteCommand::complete()
3. Using CompleteCommand as a subcommand using the derive macro.

Internal completion flow

1. Completions are triggered (see above), but at some point it ends up at CompleteCommand::try_complete()
2. try_complete either triggers the Shell's registration being printed, or the shell to handle its own completions through the Completer trait.
3. The Completer trait is a trait exposing the methods required to both complete a command and to register a shells completions
4. For Fish (Bash also implements completer, but has it's old internal completion logic still):
   a. complete(command, tokens_in_commandline) is triggered to parse the current tokens and return CompleteArgs
   b. complete_internal is a recursive function that goes through the tokens and parses them, storing already present arguments and options to filter them out of any completions provided, gets recursively called on all subcommands.
   c. in Fish::try_complete the options are returned if current starts with -- or -
   d. if value_for is set the completions for the argument are returned they are queried with completions_for_arg
   e. subcommands are returned

Fish completions

The fish completions work by taking in the tokens in the command + the current token if the cursor is in any.

Afterwards it will provide the completions as fish would normally:

1. short flags if the current token starts with -
2. long flags if the current token starts with --
3. normal values otherwise (this will not do any filtering currently, except for path completions, fish will do it for the other cases)

If you don't mind, I'd appreciate having this labeled as hacktoberfest-accepted.

Fixes #3917

[epage]

sigh need to get CI setup to properly test dynamic completions

[epage]

Since I'm prepping for the v4 release, I've gone ahead and pulled in your lint changes into #4197

[ModProg]

Since I'm prepping for the v4 release, I've gone ahead and pulled in your lint changes into #4197

makes sense.

I'm currently designing the API for fish completions. When I have them finished we could maybe have a look how a unified api could look for the different shells if that makes sense.

Or would you prefere to have them completely seperated?

[epage]

If we can have it unified, that'd be great.

Also, bpaf recently added dynamic completion support, so it might provide ideas: https://github.com/pacak/bpaf/blob/master/src/complete_run.rs

[ModProg]

I looked into how the API could look for using completions with derive/builder pattern.

The current approach in the dynamic example, to apply manually with augment_subcommands feels a bit laborious, maybe this could be done similarly to the help/version flags? Though there should also be an option to have them, but hide them from the public API displayed in --help.

But this would require it to be more tightly integrated in clap, maybe behind a feature flag, but not in a separate crate.

It could also be a trait implemented for Command, allowing it to be used like so with builder/derive:

fn main() {
    // derive
    Opts::complete();
    Opts::complete_with_flag("cmp");
    dbg!(Opts::parse());
    // builder
    command().complete();
    dbg!(command().get_matches());
}

[epage]

I looked into how the API could look for using completions with derive/builder pattern.

This seems separate from the discussion of fish, right? Can we break this out into a separate issue?

[ModProg]

I looked into how the API could look for using completions with derive/builder pattern.

This seems separate from the discussion of fish, right? Can we break this out into a separate issue?

Makes sense, but as this API is currently part of the bash implementation, it would be part of a shared API.

But I'll prepare the fish-"backend" here and leave the enduser API for a separate issue.

[ModProg]

So AFAICT fish would only need a function it can call with the current line+cursor position and get all the available completions where:

1. Options are only returned if current token is an option (i.e. starting with -)
2. Help info is return tab separated from the value
3. Short options are combined (i.e. if the user typed -r| and triggered completion, it should return -rs\tthelp for -s)

We would not need to do any filtering on this, clap does that already.

With these, completion could be called like this:

complete -x -c my_app -a "my_app complete --commandline (commandline) --cursor (commandline -C)"

[ModProg]

This implements dynamic completions for fish now, and I put some shell agnostic completion logic in the dynamic module, that should deal with all the intricacies of parsing arguments correctly (multiple values, optional before subcommand etc.)

[ModProg]

One thing that I'm not certain about is, if my handling of OsStrings due to utf8 incompatible strings is ideal or needs to be reworked.

[epage]

This is a bit difficult to review atm. Could you make the following changes so I can better see whats going on?

• Document in the PR description why this couldn't follow the bash model for completions
• Document in the PR description how these completions work
• (once we've resolved any questions on the above) Separate out each refactor into its own commit
  • Code moves should happening in the same commit as changing APIs makes it hard to see how the APIs changed
  • Some of those APIs created for the refactor changed through the PR and those changes are tied into with feature work
  • Feature work seems to be split across multiple commits
  • Documentation for parts is added later, making it harder to figure out why some trait APIs exist as they do (especially any API with _or_ in the name)

[ModProg]

I tried documenting it in the description. And you're right, I mixed these commit up a bit and will clean them up in the next days.

[ModProg]

Cleaned up my commits a little bit.

[epage]

Cleaned up my commits a little bit.

They still mix moving major sections of code with changing those sections of code. Those at least should be split out. Even better if its split out more to show step by step what changes are made.

Not that its the most important but also this would be a "refactor" and not a "feat".

[epage]

I didn't follow the bash model for completions because I didn't fully understand how it worked, and wanted to have the CompletionContext as a way for the shell specific part to handle how it selects the completions.

I would recommend we work together to figure this out. I want to start from the position of all completers being the same with any differences being handled in the registration script. Any stepping back from this ideal should be with a documented understanding of why the general design couldn't work and what is the minimal step back we can make, preserving as much of what we are trying to accomplish. There might hit a breaking point where we have to move to a model like this but I want us to learn and document why a unified model does not work.

Use the Completable trait, it supports both methods to just trigger completions and a direct replacement for Opts::parse() that will do completions or parse.

This isn't explaining the "why". I'm seeing trait methods that are unused and its unclear why they exist.

Oh, is it for derive integration? That looks to be a separate feature, unrelated to fisxh support. Let's split that out into its own Issue followed by a PR once we have an agreement on it.

[ModProg]

I would recommend we work together to figure this out.

That sounds like a good idea, what method would you prefer? Written discussion here or a voice call?

Oh, is it for derive integration? That looks to be a separate feature, unrelated to fisxh support. Let's split that out into its own Issue followed by a PR once we have an agreement on it.

Right, should at least have put that in a separate commit.

[ModProg]

Moved derive api support to #4356

[epage]

That sounds like a good idea, what method would you prefer? Written discussion here or a voice call?

First, this isn't a focus area which will require a lot more groundwork from you on this.

Second, let's start with a hackmd and then move to a phone call, if needed.

[ModProg]

I started writing the fish requirements into the hackmd, on deciding what fish would need additionally, I was unsure about what the current completion implementation actually provides and what all the flags mean.

Do you have any documentation you could point me to, on what the current API actually looks like?

[epage]

I've added some notes. As I said though, this is something you'll have to drive including how to harmonize the two systems or to be able to provide a good case for why they can't be harmonized.

[ModProg]

This could be a shared interface that at least fish and bash could use.
For bash this would allow the same output as currently, and it would support the full support for the fish shell to be on par with our static completions.

Copied from hackmd:

Proposed shared interface

This interface would allow the dynamic bash completions to be the same as it is currently, and support almost the full fish feature

Necessary flags

• separator character used to seperate returned completions e.g. \n for fish and \013 for bash. (Maybe make newline the default)
• current the word the curser is on when triggering completions, leave empty when cursor not on word e.g. COMP_WORDS[COMP_CWORD] for bash and commandline --current-token in fish
• preceding all tokens making up the command preceding the token containing the cursor e.g. COMP_WORDS[@]::COMP_CWORD for bash and commandline --current-process --tokenize --cut-at-cursor for fish
• help-seperator seperater to put between completion item and help/about text \t for fish (Optional if not set no help will be returned)
• short-flag-joining support joining multiple short flags via completions i.e. will return -ra as a completion if the current token is -r

starts with dash condition for flags

decision in clap

• flags-require-dash only return short flags when current token starts with - and long flags if current token starts with --

decision in shell

• show-short e.g. for fish commandline --current-token | string match -- "--*"
• show-long e.g. for fish commandline --current-token | string match -r -- "^-[^-]"

Shell native like path completions

Emulate in clap

Probably not a great idea as e.g. fish does quite a lot here:

• expand globs: */ba would complete to */bash.rs and */banana.rs (given the paths a/bash.rs and b/bash.rs)
• complete substrings (not only prefixes) rc/he/ba would complete to src/shell/bash.rs.

pass path completions in

Prefered, but requires us to allow * in the paths returned from fish

• paths optional, completes current token as path if not present we do our own basic path completions e.g. for fish complete -C "__fish_complete_path "(commandline --current-token) (ideally this would be __fish_complete_path (commandline --current-token) but that is currently not supported fully: fish-shell/fish-shell#9285)
• paths-allow-fish-globs allows * and ? in paths used for completions (only has impact when paths is provided), only necessary to implement filtering could be skipped at first.

have a complete files/dirs query flag

Not ideal as it removes us from the ability to filter the results

• should-complete-path exits with 0 when the next argument should be a path

Potential flags

These are the flags the current bash implementation taking, but don't have any implementation yet. I'd skip them for now.

• comp-type
• space

[ModProg]

@epage I tried to design an interface that could be shared between fish and bash allowing to move all shell specific logic into the completion script.

Do you have input on this?

[ModProg]

• paths optional, completes current token as path if not present we do our own basic path completions e.g. for fish complete -C "__fish_complete_path "(commandline --current-token) (ideally this would be __fish_complete_path (commandline --current-token) but that is currently not supported fully: fish-shell/fish-shell#9285)

with fish-shell/fish-shell@4a8ebc0 this could now be changed to __fish_complete_path (commandline --current-token) but should probably stay as is for the foreseeable future due to how some distros update their packages.

[ModProg]

@epage Just remembered that this was still open. Was there any thought put into dynamic completions in the meantime?

[epage]

After discussing this further with others interested in this effort, I've been thinking that trying to define registration/completion communication through a more format channel of CLI args is messy. I've created #5018 which instead uses env variables for this. This makes the end result for multiple shell support a hybrid between this PR and the pre-generated / static completions.

Thoughts?

[epage]

btw we now have a way to test completions. See #5026

[ModProg]

Replaced by #5048