BrianPugh / cyclopts

Intuitive, easy CLIs based on python type hints.
Apache License 2.0
299 stars 7 forks source link

[Feature Request] Completion? #89

Open Pack3tL0ss opened 8 months ago

Pack3tL0ss commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

BrianPugh commented 8 months ago

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

mattangus commented 8 months ago

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

BrianPugh commented 8 months ago

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 commented 8 months ago

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.

alextremblay commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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:

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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

BrianPugh commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago
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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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

BrianPugh commented 1 month ago

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