kotlinx-cli icon indicating copy to clipboard operation
kotlinx-cli copied to clipboard

Current API seems inconsistent

Open anti-social opened this issue 5 years ago • 14 comments

Processing options with sub-commands looks curiously. ArgParser.parse method without sub-commands just parses options. But sub-commands require different code organization.

I would suggest something that looks like:

class Opts : Options {
    val verbose by option(ArgType.Boolean, "verbose", "v", "Be verbose")
    val command by subcommands(Summary(), Multiply())
}

sealed class Subcommand(name: String) : Options(name) {
    class Summary : Subcommand("summary") {
        val invert by option(ArgType.Boolean, "invert", "i", "Invert results").default(false)
        val addendums by argument(ArgType.Int, "addendums", description = "Addendums").vararg()
    }
    class Multiply : Subcommand("multiply") {
        val numbers by argument(ArgType.Int, description = "Addendums").vararg()
    }
}

// Usage
val opts = Opts.parse(args)
val command = opts.command
val result = when (command) {
    is Subcommand.Summary -> {
        val result = command.addendums.sum()
        if (command.invert) -1 * result else result
    }
    is Subcommand.Multiply -> {
        command.numbers.reduce{ acc, it -> acc * it }
    }
}

Such an API will make possible definition of nested sub-commands.

anti-social avatar Nov 24 '20 15:11 anti-social

Such an API will make possible definition of nested sub-commands

Now you also can use nested subcommands. I didn't get what you can't do now. Could you describe a problem with current approach?

LepilkinaElena avatar Nov 24 '20 16:11 LepilkinaElena

Is it possible to describe cli options like git or docker have?

docker image ls
git remote add

anti-social avatar Nov 24 '20 17:11 anti-social

val parser = ArgParser("git")

        class Remote: Subcommand("remote", "Remote") {
            override fun execute() {
            }
        }
        class Add: Subcommand("add", "Add") {
            override fun execute() {
                println("Add")
            }
        }
        val remote = Remote()
        val add = Add()

        remote.subcommands(add)
        parser.subcommands(remote)

LepilkinaElena avatar Nov 25 '20 09:11 LepilkinaElena

Oh, missed Subcommand::subcommands method.

But anyway how can we access to Remote's options from the Add command?

Made a PoC how I think it could be done: https://gist.github.com/anti-social/22830244ada08c539956921e977b252b Such an API can be used in suspend functions. Also you will have all the options inside a single object.

anti-social avatar Nov 25 '20 11:11 anti-social

But anyway how can we access to Remote's options from the Add command?

Why do you need to have such an access?

LepilkinaElena avatar Nov 25 '20 11:11 LepilkinaElena

For example Rust's most popular libraries collect all options into a single struct:

  • https://github.com/clap-rs/clap#using-derive-macros
  • https://github.com/google/argh (see last example)

I think it's much more flexible API.

anti-social avatar Nov 25 '20 11:11 anti-social

Why do you need to have such an access?

There can be global options. Also every command can have its own options.

anti-social avatar Nov 25 '20 11:11 anti-social

There can be global options. Also every command can have its own options.

You can have global options in the main parser and options for each subcommand on any level. Could you provide a real usecase when option from remote is needed in add and it isn't enough to work with them in execute?

LepilkinaElena avatar Nov 25 '20 11:11 LepilkinaElena

You can have global options in the main parser and options for each subcommand on any level.

As I understand it is possible only if all global options and subcommand classes are defined in the function scope. Cannot find this as a nice solution.

Could you provide a real usecase when option from remote is needed in add and it isn't enough to work with them in execute?

I don't have a real use case. This is just an example that API could be more flexible.

What I really need is parsing arguments inside a suspend function.

IMHO parser shouldn't execute commands. It should only parse arguments.

anti-social avatar Nov 25 '20 12:11 anti-social

Feel free to close the issue if you have different opinion.

anti-social avatar Nov 25 '20 12:11 anti-social

We're open to any ideas. Subcommands are marked as Experimental, but to make some decisions about design we need some use cases that should be cover.

LepilkinaElena avatar Nov 26 '20 06:11 LepilkinaElena

I'll try to collect advantages of this approach. IMHO design used in Rust libraries (see examples above) allows:

  • much easier testing
  • option and command definitions at package level, not in a function scope
  • no limits how to use it
  • parsed result is just a data; you can change it, convert, serialize, store or pass someone
  • unified API with/without sub-commands

I know in Rust all the power of macroses are engaged for that. In Kotlin it's not an option but I still believe it could be done in a nice API.

anti-social avatar Nov 26 '20 10:11 anti-social

This could definitely make good use of kotlinx-serialization.

Dominaezzz avatar Dec 16 '21 09:12 Dominaezzz

I have a couple questions about the example, but in general I like the direction.

I'm a bit confused in the code example by Options vs Options(name). The former is obviously an interface providing the context for the options, but doesn't provide an actual implementation, so I'm not sure how that would function. Is this meant to be a class?

The latter is an implementation that's tied to the name of the subcommand, but that seems like adding a model inconsistency back in?

How are nested subcommands defined in this model? For instance Blah sub1 sub2 sub3 sub4 arg arg arg

Is there a root command, or is this a homogenous tree of subcommands? Is this suggesting dropping automatic execution facilities entirely, or is this simply not shown?

Are we still able to automatically generate help blurbs with this structure?

BenjaminHolland avatar Dec 19 '21 20:12 BenjaminHolland