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

[Feature Request] Completion? #89

Open
Pack3tL0ss opened this issue Jan 25, 2024 · 25 comments
Open

[Feature Request] Completion? #89

Pack3tL0ss opened this issue Jan 25, 2024 · 25 comments
Labels
enhancement New feature or request v3

Comments

@Pack3tL0ss
Copy link

Curious if this project currently has completion (couldn't find it in the docs or code) or if there are aspirations to provide completion in the future??

@BrianPugh
Copy link
Owner

I would love to add autocompletion, but have no experience in the area. I would certainly welcome any input, whether it be general recommendations, library suggestions, PRs, etc.

@BrianPugh
Copy link
Owner

Just a heads up, I am working on this, hoping to have something testable in the next week or two.

@BrianPugh
Copy link
Owner

shell completion is infinitely more complex than I was initially expecting 😄 . Still a WIP.

@mattangus
Copy link

mattangus commented Feb 19, 2024

This is somewhat of a deal breaker for me so I'm keen to help out to add this feature!

@BrianPugh
Copy link
Owner

Currently I'm working on adding bash and zsh support; i'm taking another direction on it and going to be basing the code off of shtab's implementation, which I think is a good reference.

Originally I was pursuing the idea of adding functionality to generate an argparse.ArgumentParser object from a cyclopts.App. However, some Cyclopts concepts don't directly translate (such as positional-or-keyword arguments). The benefits of this approach is that other off-the-shelf tooling could be used. I'm still considering this approach, but it does have its issues.

@BrianPugh
Copy link
Owner

BrianPugh commented Feb 19, 2024

I'm still thinking about this; I'm actually leaning towards the argparse solution (and maybe even also having a to_click method later) where I map postional-or-keyword arguments to be position-only to make them more compatible with those libraries/ecosystems.

@BrianPugh BrianPugh added the enhancement New feature or request label Mar 6, 2024
@alextremblay
Copy link

Hi @BrianPugh

I've recently discovered cyclopts after becoming disillusioned with typer's slow (possibly stalled) development

I love everything you've done with it so far, and the only thing preventing me from adopting it in all my projects is the lack of shell completion. I have users leveraging shell completion in typer now, and taking that away from them is a hard no

I'm wondering if you'd be willing to publish your WIP on shell completion in another branch so that I or others could take a look at it and possibly help you finish it?

@alextremblay
Copy link

nvm, I just realized you already did that! hahaha

looking at the code, i think i see what you were going for. I agree that some kind of parser transformer (converting and App to an ArgumentParser or a click Group would be better

@BrianPugh
Copy link
Owner

Hey @alextremblay! I totally agree that shell completion should be the next important feature to implement

I'm actually working on a cyclopts v3.0.0 that does a fairly large revamp of all the internal structures. The big feature of v3.0.0 is that it will support dictionary-like parameters (dicts, typeddicts, dataclasses, attrs, pydantic, etc). Error messages will also be much more helpful because exact user-provided values and their source (CLI, config files, environment variables, etc) are now explicitly logged. Some of the lesser used cyclopts functionality will have a slight API change, but I don't think it'll impact most users.

In cyclopts v3.0.0 a lot more of the argument parsing/resolving is done up-front, which should make implementing shell-completion much easier. My current branch I'm working on is here. It's not functional yet, and a lot of the commits are sloppy due to the large changes. It's also going to change a lot, but you can track my progress a bit over there.

My initial attempts were converting cyclopts structures to something like ArgumentParser objects, but there are some incompatibilities that make it not work, so I sort of abandoned that idea. Benefits of that approach is that it could increase inter-operability with other CLI libraries. However, for now, I'll just be implementing direct shell completion without intermediate conversion.

I've been diligently working on v3.0.0, but no guarantees on a timeframe. I hope to have it done in about a month (and then start working on shell completion again), but you never know if life gets in the way 😄 .

@alextremblay
Copy link

alextremblay commented Aug 17, 2024

That’s awesome!

Another option you may want to consider regarding shell completion, outsourcing!
I've been keeping an eye out for a while now on a tool called carapace-bin
It comes from the land of golang, but has since grown to encompass rust and python CLI frameworks.

The downside of using carapace-bin instead of doing your own shell completion Is that all users who want shell completion would have to install carapace (more on that later)
The upsides are many:

  • shell support; carapace supports many more shells than any existing python CLI framework in existence, and certainly more than we can hope to support with an initial implementation
  • Extensive type completion; carapace supports a dizzying array of different kinds of completion (file completion, folder completion, completion of files with a specific extension, completion of files/folders in a specific path, as well as an extensive array of other completion macros
  • Performance; carapace-bin is a single-file go binary, it loads up custom Specs on shell startup. When it’s tasked with generating completions for a python CLI, it’s blazing fast. Far faster than any python-based shell completer could ever hope to be, no matter how well optimized
  • I just think it’s really neat :)

The core idea here is that we could pre-generate completion specs for all the parts of our cyclopts command tree (including subcommands, options, positional args) which are known at compile-time, and compile them into an installable carapace-spec. For parts of the cyclopts CLI that are too dynamic to pregenerate, we have two options:

  1. We could insert a “stub” in the carapace spec, what carapace calls an exec macro… (side note, there’s a lot of useful info on how we could use this here in this issue I opened
  2. Less preferable, but still viable, we could implement completion logic in cyclopts similar to how click does it (looking for a _{cli name}_COMPLETE env var to tell us we need to parse sys.argv and generate completions) and then use carapace-bin’s Click bridge

We could also provide a mechanism for users to install carapace and install our CLI spec file

i guess all of this is to say: if you like this idea, I’d like to work with you to implement carapace-bin completion in cyclopts. Once the v3 codebase is semi-stable (to the point where the data structures involved are mostly stable), I can start working on a program that takes those data structures and generates a carapace Spec file.
closer to the release of v3, or after v3 release, I can work on integrating this spec generator into cyclopts itself

Thoughts?

@BrianPugh
Copy link
Owner

I like the idea! I'll post back in this thread in a week or two with a more meaningful follow up (with a hopefully solidified v3). I would love help on this!

@arogozhnikov
Copy link

@BrianPugh maybe shtab's developers would be interested in incorporating positional-or-keyword arguments?
It is a very pythonic way of treating arguments after all.

I feel like shtab is feature-rich, focused and not bloated.

cyclopts api + autosuggestion would be awesome.

cc @casperdcl

@casperdcl
Copy link

casperdcl commented Aug 21, 2024

What's a "positional-or-keyword" arg?
app (<arg> | --arg=<arg>)?

@BrianPugh
Copy link
Owner

Hey @arogozhnikov @casperdcl !

Big fan of shtab, I was actually going to base Cyclopt's autocompletion on it; the codebase is very clean and minimal. Is there an API that Cyclopts could use (or vise-versa) to hook into shtab directly?

What's a "positional-or-keyword" arg?
app ( | --arg=)?

Exactly. Something like:

@app.default
def foo(src, dst):
  print(f"Copying {src} -> {dst}")

Has the following equivalent CLI invocations:

python my-script.py fizz buzz
python my-script.py --src fizz buzz
python my-script.py --src fizz --dst buzz
python my-script.py --dst buzz --src fizz
python my-script.py --dst=buzz fizz

@casperdcl
Copy link

casperdcl commented Aug 21, 2024

I... don't think argparse supports this.

from argparse import ArgumentParser
parser =  ArgumentParser(prog="test")
parser.add_argument('arg', default='pos', nargs='?')
parser.add_argument('--arg', default='opt')

parser.parse_args()  # ignores --arg

Though the two cases will exist in the parser object metadata so presumably could be handled by shtab.

@BrianPugh
Copy link
Owner

I... don't think argparse supports this.

Correct, argparse does not. It's actually the primary reason I gave up attempting to create a cyclopts->argparse converter.

@casperdcl
Copy link

casperdcl commented Aug 21, 2024

Just tested; the shtab aspect seems to still work(ish):

#!/usr/bin/env python
#test
from argparse import ArgumentParser
import shtab

parser = ArgumentParser(prog="test")
key = 'arg'
vals = 'foo bar baz'.split()
parser.add_argument(key, choices=vals, nargs='?')
parser.add_argument(f'--{key}', choices=vals)
shtab.add_argument_to(parser)
parser.parse_args()
eval "$(test --print-completion bash)"
test <TAB> # foo bar baz
test f<TAB> # foo
test -<TAB> # --arg
test --arg <TAB> # foo bar baz
test --arg foo <TAB> # foo bar baz

that last line ambiguity btw is probably why argparse doesn't let you do this.

I don't think python my-script.py --src fizz buzz is easy to support. And it also doesn't seem advisable for any CLI tool to support for that matter. Here be dragons/footgun material?

@arogozhnikov
Copy link

I don't think python my-script.py --src fizz buzz is easy to support.

I also found this counterintuitive: python doesn't allow such calls; and when refactoring code I'm thinking about which python calls can be/should be broken when I change function signature, and this CLI call wouldn't cross my mind.

@BrianPugh
Copy link
Owner

BrianPugh commented Aug 22, 2024

Here be dragons/footgun material?

So this is an excellent time to discuss this, because if we're going to make a breaking change, it's best to do it with the upcoming v3.

As a thought experiment, let's partially implement the cli tool cp with the recursive -r flag. The following cli invocations seem reasonable:

cp -r source_path destination_path
cp source_path destination_path -r
cp source_path --dst destination_path -r
cp --src source_path --dst destination_path -r
cp --dst destination_path --src source_path -r

The following definitely seems wonky and unintuitive:

cp -r --dst destination_path source_path
cp --dst destination_path -r source_path
cp --src source_path destination_path -r

A reasonable python function signature would look like:

@app.command
def cp(src, dst, recursive: Annotated[bool, Parameter(name="-r")]):
    ...

To match python-syntax, we could simply say that positional-arguments cannot come after keyword arguments. However, in CLI it's quite frequent to front-load the command with a bunch of flags/keywords with the positional arguments trailing. It's also common to put all the flags/keywords after the positional arguments (at the end). It is NOT common to inter-mingle the two.

So, a first attempt at fixing this could be the following ruleset (using inspect.Parameter.kind terminology):

  1. KEYWORD_ONLY and VAR_KEYWORD arguments can go anywhere.
  2. Once a POSITIONAL_OR_KEYWORD is provided as a keyword, ALL subsequent arguments must be provided as keywords/flags.
  3. POSITIONAL_ONLY and VAR_POSITIONAL arguments can go anywhere. However, because of (2) they must be before any POSITIONAL_OR_KEYWORD arguments have been specified-by-keyword.

I am tempted to add a fourth rule: "Boolean POSITIONAL_OR_KEYWORD don't force subsequent POSITIONAL_OR_KEYWORD parameters to be keyword-only, until a positional argument would have collided with the boolean parameter." However, I think this is a little too complicated, and people should be making their boolean flags as KEYWORD_ONLY.

So, for our example, the function signature should actually be:

@app.command
def cp(src, dst, *, recursive: Annotated[bool, Parameter(name="-r")]):
    ...

Thoughts/Feedback on the above ruleset?

@arogozhnikov
Copy link

arogozhnikov commented Aug 22, 2024

I think equivalent of your procedure would be:

  1. parse inputs to positional and kwargs (I assume this is possible, but maybe that's non-trivial)
  2. move all kwargs to right, while preserving order of args
    -r input1 --flag 1 --otherflag 2 input2
    input1 input2 -r --flag 1 --otherflag 2
    
  3. normal python call

I likely oversimplify, I just want some very simple 'visual rule' how CLI args convert to function inputs.
If above interpretation is somewhat correct, then it sounds very reasonable for me.

@casperdcl
Copy link

casperdcl commented Aug 22, 2024

python my-script.py --src fizz buzz
python my-script.py buzz --src fizz

seems as borked as:

def foo(src, dst):
    ...
foo(src="fizz", "buzz")
foo("buzz", src="fizz")

let's partially implement the cli tool cp

I disagree with your implementation. AFAIK cp only has positional src args, and the last one is treated differently depending on options, and such logic is handled manually by the program. So the equivalent should be:

def cp(*source, target_directory=None, no_target_directory=False, recursive=False):
    if target_directory is None:
        source, target_directory = source[:-1], source[-1]
    dest = target_directory if no_target_directory else None
    assert xor(dest, target_directory)
    copy = getattr(shutil, 'copytree' if recursive else 'copy2')

cp("foo", "bar")
cp("foo", target_directory="bar")
cp foo bar
cp foo --target-directory=bar

Meanwhile all of the following raise errors:

cp("bar", source="foo")
cp("bar", source=("foo",))
cp(source="foo", target_directory="bar")
cp(source=("foo",), target_directory="bar")
cp --source foo bar
cp --source foo --target-directory=bar

@BrianPugh
Copy link
Owner

seems as borked as:

Exactly, which is why I'm proposing to change how the arguments are parsed to disallow these unusual/unintuitive situations.

I disagree with your implementation.

It was meant to be a simple example showing intermingling of flags and positional-or-keyword parameters.

Meanwhile all of the following raise errors:

And I agree, I think all of those should be disallowed.

@BrianPugh
Copy link
Owner

BrianPugh commented Aug 22, 2024

@arogozhnikov They're similar, but we have to parse keywords first due to nuances with the number of tokens that get associated with each python parameter. Consider the following:

def move(x:int , y: int, *, origin: tuple[int, int] | None = None):
    """Move player location."""
move --origin 3 4 1 2 
# Parsed as:
    origin=(3,4)
    x=1
    y=2

We first have to parse --origin and figure out that we need to consume 2 tokens for it. Only then can we figure out that 1 and 2 are for x and y, respectively.

EDIT: I may have mis-interpretted your comment; I think we're all saying the same thing.

@casperdcl
Copy link

fyi argparse would expect move --origin 3 --origin 4 1 2

@BrianPugh
Copy link
Owner

Typer/Click works the same as Cyclopts here (consuming enough tokens to populate the tuple). I think it's the more intuitive option, and also allows you to do list of tuples like:

@app.default
def main(coordinates: List[Tuple[int, int]]):
    ...
python my-script.py --coordinates 1 2 --coordinates 3 4

@BrianPugh BrianPugh added the v3 label Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v3
Projects
None yet
Development

No branches or pull requests

6 participants