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

Support classes #82

Open
jordan-schneider opened this issue May 19, 2020 · 9 comments
Open

Support classes #82

jordan-schneider opened this issue May 19, 2020 · 9 comments

Comments

@jordan-schneider
Copy link

jordan-schneider commented May 19, 2020

All of my commands have some common setup and teardown. One paradigm for this is to have a class with an __init__() and __del__() method, and then have each command by a method of that class. fire supports this, for example. I don't know how difficult this would be, but it would save me a lot of repetition if I could pass a class to defopt.run() instead of a list of functions.

@anntzer
Copy link
Owner

anntzer commented May 19, 2020

I agree the API provided by defopt is not so great when multiple commands share the same parameters. The solution I've been thinking about is to support dataclasses, e.g.

@dataclass
class Foo:
    x: int
    y: float


def somefunc(foo: Foo): ...
def otherfunc(foo: Foo): ...

defopt.run([somefunc, otherfunc])

The main issue how to decide on exact argument name conversion rules (if foo is positional-or-keyword, do we just splat x and y as positional CLI arguments? if foo is keyword-only, are the flags named --foo-x/--foo-y or just --x/--y? what if dataclasses support some day keyword-only args (https://bugs.python.org/issue33493)?). I haven't really had the opportunity to think what's best here.

I guess instead of dataclasses we could also support "any class whose constructor is type-hinted in a defopt-compatible way", similarly to how we already support "any class whose constructor takes a single str" (but the latter just drops the name of the parameter in the class constructor, so we're going to have some inconsistencies then...).

I don't know anything about fire, but from a quick look there's a lot of things that I would definitely consider as out of scope (e.g. the "grouping commands" or "accessing properties" or "chaining function calls" sections of the guide). Can you clarify what parts you are exactly looking for?

@jordan-schneider
Copy link
Author

jordan-schneider commented May 19, 2020

dataclasses would be an improvement, but I would still have to repeat the setup and teardown. Let me be more specific. I currently have to do this:

def f(x: int, y: str, z: pathlib.Path) -> None:
    state = read_state(path)

    # Do some processing

    write(x, y, path)

def g(x: int, path: pathlib.Path) -> None:
    state = read_state(path)

    # Do something else

    write(x, path)

if __name__ == "__main__":
    defopt.run([f, g])

The syntax that fire supports, which removes all this repetition, is

class Command:
    def __init__(self, path):
        self.state = read_state(path)
        self.path = path

    def f(x: int, y: str) -> None:
        # stufff
    def g(x: int) -> None:
        # other stuff

    def __del__(self):
       write(self.state, self.path)

if __name__ == "__main__":
    defopt.run(Command)

And then execution would just use both the __init__ arguments and the function arguments

python file.py f --x 5 --y hello --path ~/mypath/myfile

Is that clear?

Thank you for your promptness.

@anntzer
Copy link
Owner

anntzer commented May 19, 2020

I got that from fire's doc, but if that's what you want, why don't you just use fire? In other words, what benefits do you expect from using defopt instead?

@jordan-schneider
Copy link
Author

jordan-schneider commented May 19, 2020

defopt handles list arguments/options sanely. If you want to have a list option in fire, you need to write it as a python style string

In fire:
python file.py --list '[a,b,c]'
In defopt:
python file.py --list a b c

AFAICT the only other API CLI generator that does this well is argh, but argh requires you to manually annotate list parameters.

@anntzer
Copy link
Owner

anntzer commented May 19, 2020

I guess what you can do right now is something like

from typing import Literal


class Command:
    cmd: Literal["foo", "bar"]
    x: int
    y: float

    def __init__(self, cmd, *, x, y):
        self.x = x
        self.y = y
        getattr(self, cmd)()

    def foo(self):
        print("foo", self.x, self.y)

    def bar(self):
        print("bar", self.x, self.y)


if __name__ == "__main__":
    import defopt
    defopt.run(Command)

(which works).

The problem I have with the class-based API is that you're basically writing a class solely for the purpose of providing a command-line API, whereas I feel that defopt's philosophy is that the entire module should still make sense (the functions should be callable with a normal pattern from python code, etc.) if defopt was removed (this should only remove the CLI) (Yes, I know, with class-based APIs, you could write Command(x, y).foo() but... the API feels clunky). (There's also the additional point that defopt is rather fundamentally oriented around mapping keyword-only args to CLI flags and postitional(-or-keyword) args to positional CLI args.)

@jordan-schneider
Copy link
Author

(I have switch to using argh because I decided I really wanted the full power of argparse and am willing to pay the cost of writing decorators. Feel free to close this issue if you don't care about setup and teardown.)

Yeah, I can how forcing the user to write a class just for defopt kind of breaks the philosophy of the thing. I think you could get the same results by only using functions if you allowed people to specify multiple sets of functions to dispatch.

def setup(...):
  ...

def teardown(...):
  ...

def f(...):
  ...

def g(...):
  ...

if __name__ == "__main__"
  import defopt
  defopt.run(setup)
  defopt.run([f, g])
  teardown()

And then the CLI would have arguments/flags for both setup and one of f or g. The main difficulty I see here is how to handle getting the results of setup to f or g. You'd have to allow some kind of piping or people would be forced to shove things into global variables.

@anntzer
Copy link
Owner

anntzer commented Jun 2, 2020

The case of teardown can also be handled by an atexit handler (although again you'd need globals to pass things around -- but once you're architecting your module for CLI use anyways I don't think globals are that bad as you're effectively assuming a single-use pattern).

I'll leave this open mostly because I would personally like some kind of dataclasses support, but again haven't managed to decide on an API yet.

@hhoeflin
Copy link

I just found this library and I love its general approach, also how it extracts docs from the docstring for the argument. Much better approach I feel than typer! But common arguments are still a gap I see and classes would be great to fill it.

You mentioned earlier that there would be issues with deciding on what to name the shared arguments and how to handle them. I think one solution would be to require that all shared arguments are specified before the subcommand (this is how typer does it, just more clunkier with 'callback' and 'ctx' objects). Then all standard rules for functions that you already have apply for the shared args, and the signature of the subcommand doesn´t have to change at all.

Anyway, great library, I will switch to it from using typer as it feels like easier to re-use things.

P.S. Writing a small class just for the sake of defining the API does not feel bad. That can be kept very short.

@anntzer
Copy link
Owner

anntzer commented Aug 30, 2022

Passing common arguments before the subcommand is an interesting idea. Can you be more explicit as to the API you propose, though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants