Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autocompletion #13

Open
Tyrrrz opened this issue Aug 14, 2019 · 22 comments
Open

Autocompletion #13

Tyrrrz opened this issue Aug 14, 2019 · 22 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@Tyrrrz
Copy link
Owner

Tyrrrz commented Aug 14, 2019

Investigate what's possible

@Tyrrrz Tyrrrz added the enhancement New feature or request label Aug 14, 2019
@domn1995
Copy link
Contributor

domn1995 commented Apr 21, 2020

This looks like a promising tutorial on how to do it for bash: https://iridakos.com/programming/2018/03/01/bash-programmable-completion-tutorial

The tricky part is we would need the completion script to be copied to a user's bash directory at the time of installation of the CLI tool. I don't think that functionality belongs in CliFx, but we can definitely provide a method of generating the completion script from the command schema and leave it up to CliFx developers to bundle it with their app or not.

@Tyrrrz
Copy link
Owner Author

Tyrrrz commented Apr 21, 2020

Yeah I seem to remember that Windows had a similar setup where the user had to configure something once.

@Tyrrrz Tyrrrz added the help wanted Extra attention is needed label May 11, 2020
@Tyrrrz
Copy link
Owner Author

Tyrrrz commented May 11, 2020

So it seems that we will indeed need to configure it once per shell.

Suggested design:

When the CliFx application executes for the first time, run appropriate scripts for each supported shell (PowerShell, Bash, ...?) to register auto-completions. Since it's quite a heavy operation and potentially damaging, make it disabled by default, gated by .EnableAutoCompletion() on CliApplicationBuilder.

@domn1995
Copy link
Contributor

domn1995 commented May 12, 2020

Looking for a little more info on the EnableAutoCompletion() API and throwing out some ideas.

  • What shells do we generate for?
    • All supported shells?
    • Only for those installed on the user's system?
    • Or do we let the CliFx consumer configure this behavior? Maybe through a AutoCompletionOptions object that CliFx users can configure and extend?
  • Do we want to support CliFX consumers extending the autocompletion functionality on their own projects or must it happen within CliFx?
  • How do we know whether we need to regenerate the autocompletion scripts or not?
    • If we regenerate every time, are we just overwriting the previous ones?
  • What about providing a built-in command to enable autocompletion like myapp install-autocompletion <shell> where <shell> are the supported shell types? I like this idea because it is explicit. By default it could install for the current shell.

@Tyrrrz
Copy link
Owner Author

Tyrrrz commented May 12, 2020

What shells do we generate for?

Good question. Ideally for the currently running shell if it's reasonably practical to get that information. Otherwise I guess just try to enable for all supported shells and ignore errors in case they are not available. What shells should we support? I guess PowerShell, bash, anything else?

Do we want to support CliFX consumers extending the autocompletion functionality on their own projects or must it happen within CliFx?

I don't think it makes sense to extend it on consumer's end.

How do we know whether we need to regenerate the autocompletion scripts or not?

That might be a problem. An alternative approach would be to add a new directive [enable-autocompletion] that would do it on-demand. Maybe let the user know about it in --help.

What about providing a built-in command to enable autocompletion like myapp install-autocompletion where are the supported shell types? I like this idea because it is explicit.

I would prefer a directive for that because that's the interface for cross-cutting concerns.

@domn1995
Copy link
Contributor

domn1995 commented May 12, 2020

What shells should we support? I guess PowerShell, bash, anything else?

I think supporting these two for initial feature release is perfect. Maybe Windows Command Prompt as well?

We should also design it so extending it within CliFx to add support for other shells is pretty painless. Is this a design goal that you agree with?

I don't think it makes sense to extend it on consumer's end.

Agreed. They should extend it in CliFx.

I would prefer a directive for that because that's the interface for cross-cutting concerns.

Oh yeah, agreed! Totally forgot about directive support. Another reason this project is awesome!

@Tyrrrz
Copy link
Owner Author

Tyrrrz commented May 12, 2020

😊

We should also design it so extending it within CliFx to add support for other shells is pretty painless. Is this a design goal that you agree with?

If it's possible, yes. But I think it's more important to get coverage for the most common shells than to make it extendable. Let's see how it goes and adapt.

@domn1995
Copy link
Contributor

If it's possible, yes. But I think it's more important to get coverage for the most common shells than to make it extendable. Let's see how it goes and adapt.

Agreed. I'm currently working on #17 and then I can start prototyping this.

@domn1995
Copy link
Contributor

Here's how autocomplete installation works for dotnet:
https://docs.microsoft.com/en-us/dotnet/core/tools/enable-tab-autocomplete

And here's the code from the generating the suggestions for the dotnet complete command:
https://github.com/dotnet/sdk/tree/master/src/Cli/dotnet/commands/dotnet-complete

I think emulating this is probably the way to go, and it actually seems relatively simple.

@Tyrrrz
Copy link
Owner Author

Tyrrrz commented May 15, 2020

I see, so it actually will need 2 commands, one to setup autocomplete and one to query it.

@LordLyng
Copy link

dotnet utilizes dotnet-suggest (https://github.com/dotnet/command-line-api/wiki/dotnet-suggest) in their experiemental System.CommandLine (https://github.com/dotnet/command-line-api) library.
I'm unsure if dotnet-suggest uses dotnet-complete underneath.

@Tyrrrz
Copy link
Owner Author

Tyrrrz commented Jul 28, 2020

I think the user experience where you have to install a global tool and then also configure it for your terminal manually is completely horrible. It's also probably evident by how much dotnet-suggest has been downloaded so far.

Ideally, it should be fully automatic or at least configurable directly from within the same CLI tool.

@jcotton42
Copy link

I've used dotnet suggest-powered completion before and the lag between hitting tab and getting a completion can be quite annoying.

@Tyrrrz
Copy link
Owner Author

Tyrrrz commented Feb 12, 2021

Relevant link: https://docs.microsoft.com/en-us/dotnet/core/tools/enable-tab-autocomplete

Examples on how to enable tab completion in various terminal (for dotnet CLI, but can be adapted for general case).

@mauricel
Copy link

mauricel commented Mar 2, 2021

Spiked this today. Interesting.

I added a [autocomplete] directive which works as follows:

> clifx.demo [autocomplete] b
book add
book
book list
book remove

I then registered the ArgumentCompleter with powershell to call the application using the autocomplete directive. So far, I've only toyed with commands so far, might deep dive a bit later.

@adambajguz
Copy link

Perhaps [suggest] is a better name?

@mauricel
Copy link

mauricel commented Mar 23, 2021

I'd like to make the following suggestions to progress this ticket forward.

Design suggestion 1: Support passing of auto-complete text by environment variable
eg. cli.exe [suggest] --var clifx-suggest-{guid} --cursor {cursor-position}

Powershell and Bash supply a text variable containing the unmodified autocompletion text (string), and a cursor position variable (int). Passing the text variable as a command line argument in powershell is unreliable. Powershell has breaking weirdness in the way variables are split, then passed to the CLI. I found I got better results by stuffing the text variable into an environment variable, and passing the environment variable name to in the suggest directive.

The other issue I found was that the cursor position is not usable unless we had access to the original text. I recall having issues because Environment.CommandLine doesn't provide access to the original text -- it may be escaped.

Regarding safety of environment variables: Powershell environment variables are process scoped, and can be deleted after processing. I think those should be okay. I don't think Bash environment variables support such scoping, however I think this approach should be okay. Otherwise one can fall back to just passing auto-complete text by command line.

Design suggestion 2: Fallback support passing of auto-complete text by command line

eg. cli.exe [suggest] {autocomplete-text}

The .net documentation above describes a use case (zsh) where only the text is available. I think we should also provide this as a fallback.

Design suggestion 3: Enhance CommandInput to return raw tokens
My spike has a great deal of extra code. A better design might be to have CommandInput extract raw tokens as part of its parsing process, then pass these tokens to a service to provide sensible suggestions. I wanted to seek input before modifying existing classes. See https://github.com/mauricel/CliFx/tree/%234-spike-unify-parser-code

@Tyrrrz
Copy link
Owner Author

Tyrrrz commented Mar 23, 2021

@mauricel

Design suggestion 1: Support passing of auto-complete text by environment variable

I like the idea of passing parameters via environment variables. It's cleaner and probably easier for us.

Design suggestion 2: Fallback support passing of auto-complete text by command line

Sounds good.

Design suggestion 3: Enhance CommandInput to return raw tokens

Hmm, can you explain how this will benefit us? Can we not pass parsed inputs to suggestion service instead?

Also, if we need this, can we have each input encapsulate its own token(s), so it's not kept in a separate object?

@Tyrrrz
Copy link
Owner Author

Tyrrrz commented Mar 23, 2021

@mauricel
Copy link

mauricel commented Mar 24, 2021

Hmm, can you explain how this will benefit us?

The code in CommandInput.Parse() is currently is responsible for splitting the argument array into directive, command, parameter and option components. My first naive approach simply re-implemented that behaviour (somewhat), but I didn't like how there were two (slightly different) implementations. I found bugs because I'd misunderstood how aliases worked. The benefit I was aiming for was to keep the responsibility of splitting arguments up into command, parameter, components in one place.

Can we not pass parsed inputs to suggestion service instead?

Let me explore here a bit more. Ideally, that's what I'd like to do. Yesterday, I wasn't sure how we can use CommandInput for partial string matching of Commands. I didn't like pulling command information out of the CommandInput.Parameters fields too much so I thought passing the parser's "state" to the service would be better. Upon second thought, I see an issue with having two models of the parser output that don't match.

Also, if we need this, can we have each input encapsulate its own token(s), so it's not kept in a separate object?

Yeah, that makes sense.

Thanks for the design review!

@mauricel
Copy link

... Let me explore here a bit more

Seems like we won't need it for now. Thanks for the sense check.

@Tyrrrz
Copy link
Owner Author

Tyrrrz commented Mar 24, 2021

No problem 🙂

mauricel added a commit to mauricel/CliFx that referenced this issue Mar 25, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 25, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 25, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 25, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 25, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 30, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 30, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 30, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 30, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 30, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 30, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 31, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 31, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 31, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 31, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 31, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Mar 31, 2021
@Tyrrrz Tyrrrz mentioned this issue Apr 1, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Apr 12, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Apr 12, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Apr 12, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Apr 12, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Apr 12, 2021
mauricel added a commit to mauricel/CliFx that referenced this issue Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants