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

Improve support for autocompletion: shell_complete vs autocompletion #949

Open
1 task done
tiangolo opened this issue Aug 23, 2024 · 2 comments · May be fixed by #974
Open
1 task done

Improve support for autocompletion: shell_complete vs autocompletion #949

tiangolo opened this issue Aug 23, 2024 · 2 comments · May be fixed by #974
Assignees
Labels
feature New feature, enhancement or request

Comments

@tiangolo
Copy link
Member

tiangolo commented Aug 23, 2024

Privileged issue

  • I'm @tiangolo or he asked me directly to create an issue here.

Issue Content

In Short

We want a single parameter to handle CLI parameter autocompletion, based on type annotations.

Current State

Right now the Typer docs only talk about the function for the parameter autocompletion.

And Typer has (proper) support for that function based on type annotations.

It checks the types for a Context, list[str] for other args, and str for the current incomplete args: https://typer.tiangolo.com/tutorial/options-autocompletion/#types-types-everywhere

The support for autocompletion is based on the type annotations (like the rest of Typer), not on the order or name of the arguments.

Now with Click 8.x the underlying parameter autocompletion was removed and there's a new one shell_complete (https://click.palletsprojects.com/en/8.1.x/api/#click.ParamType.shell_complete).

In Typer, autocompletion raises a warning already: https://github.com/fastapi/typer/blob/master/typer/core.py#L65

But there are no docs nor proper support for shell_complete based on type annotations.

We Want

We want to have support for completion based on type annotations. The parameter should be able to expect a Context and a str with the incomplete parameter. But I'm not sure of the parameter name for this function.

Note: we probably don't need to support list[str] for extra args as that's available from context.args.

We should have that as the main documented feature. We could have a note at the end mentioning that there's a (now deprecated) autocompletion parameter, with the basics of how it worked, but the main functionality would be with the new parameter.

Hardest Problem in CS: Naming

Writing this I'm realizing that in Typer I'm using the same parameter name that was available in Click: autocompletion, but in reality, it has different semantics.

In Click, the meaning of the parameters is based on the position/order.

In Click 7.x it had to be in the exact order of ctx, args, incomplete:

def get_colors(ctx, args, incomplete):
    ...

In Click 8.x it has to be the exact order of ctx, param, incomplete:

def complete_env_vars(ctx, param, incomplete):
    ...

In Typer, autocompletion it's based on the type annotations, which also means that the order doesn't matter, if all of the parameters are present or not doesn't matter, the name of the parameters doesn't matter. The only thing that matters is the type annotations: https://typer.tiangolo.com/tutorial/options-autocompletion/#types-types-everywhere

This means that the name of the parameter autocompletion is the same as in Click, but the semantics are actually not the same. 🤔

That makes me think that maybe the parameter should have a different name, so that people don't get confused with it assuming it's the same as in Click, and it really is not, at least not in the same way.

Maybe we should just keep the same name autocompletion just because that's the main name we've had documented in Typer. 🤔

My original thought was to support a parameter shell_complete (like the one in Click) but based on type annotations. But now thinking about it, I think maybe it makes more sense to have a different name (or just keep the old autocompletion name). And have it based on type annotations.

I think the new/final parameter should be able to request (via type annotations) the Context and the incomplete str. I don't think we should support it requesting the param Click Parameter until we find a reason to do so (I can't find any yet).

The other thing is, the old Click autocompletion supported returning str values or tuples. The new shell_complete expects objects of type CompletionItem, but I wouldn't think requiring users to import that class and create instances of it would be a great developer experience, so I would prefer to keep supporting the older syntax too.

Note on CompletionItem

From the source, I understand that the main thing it provides apart from the previously supported data is a new type that tells the completion parts if the thing to complete is a directory or file... I'm not sure in which cases that type is useful, maybe internally, but not sure for completion functions. And also, if there's a use case where that makes sense, we can probably know the type from the type annotation (e.g. Path).

@bckohan
Copy link
Contributor

bckohan commented Sep 12, 2024

After reviewing click 7/8 and how I'm using shell_complete downstream I have the following recommendations:

  1. Allow completer functions to return lists of CompletionItems, strings or 2-tuples of completions and helps. CompletionItem may change over time and there's no harm in passing them through if user functions return them. (my downstream functions already do return these)

  2. Deprecate args in favor of click.core.Parameter. The parameter class is helpful for defining generic completer
    functions that otherwise would not know which parameter they are attached to. I'm using the parameter class downstream to weed out duplicates in multi value argument completions and to know if the parameter value was a default or not:

        if (
            not allow_duplicates
            and param.name
            and ctx.get_parameter_source(param.name) is not ParameterSource.DEFAULT
        ):
            present = [value for value in (ctx.params.get(param.name) or [])]

I think args is already not backwards compatible with how it behaved in Click 7 It was also not very useful in Click 7. It resolved to the list of arguments passed through the environment variables used by the different shell completion entry point scripts and was also subject to additional ad hoc processing depending on the shell. Reverting it to the Click 7 behavior would be difficult and also probably not worth it because again, it wasn't very useful. It is also not equivalent to ctx.args.

The Parameter class in concert with the Context however gives you access to how click interpreted the complete CLI parameters before the incomplete string.

I think the best thing to do would be to allow completer functions to specify args: List[str] and/or param: click.core.Parameter, but throw a deprecation warning if args is present and pass an empty list. I really doubt theres a lot of existing code making use of args out there because its not very useful (you'd have to do a lot of manual processing in many cases) and also I think its already broken and nobody seems to be complaining. Parameter on the other hand is much more useful. So following this recommendation would not break existing code more than it already is and allow graceful transition to using Parameter instead.

@bckohan
Copy link
Contributor

bckohan commented Sep 18, 2024

There is a way to support the old args: List[str] parameter. This and the other suggestions (minus the deprecation warning) are submitted here: svlandeg#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature, enhancement or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants