Suspending command support about clikt HOT 16 CLOSED

Previous discussions are in #98 and #185

from clikt.

Thanks for the links, Jake. To summarize the previous discussions:

Making main a suspend fun is slightly nicer for folks who use coroutines, and slightly worse for anyone who doesn't. It's easy to add your own runBlocking call, so I haven't built it in.

Correct me if I'm wrong, but there's isn't any functionality gained my providing a suspend fun run, it just looks nicer?

If Clikt made run suspend, then either CliktCommand.main would have to be suspend as well, in which case you still have to add a runBlocking to your code somewhere (or make your top level main be suspending), and now people who don't use coroutines have to deal with that. Or CliktCommand.main stays non-suspending, which means Clikt itself would have to execute the coroutines, and since JS doesn't have runBlocking, I'm not sure how that would even work.

I guess we could provide SuspendingCliktCommand as an option, but then we'd need to add kotlinx.coroutines as a dependency (does anyone care about that?) or publish a separate module with just that class. And class C: SuspendingCliktCommand() { override fun runSuspend() {} } is like two more characters to type than class C: CliktCommand() { override fun run() = runBlocking {} }, so are we really gaining anything?

from clikt.

If you propagate the suspend into the main API, you wouldn't need the additional dependency (aside from in testing).

I think the big advantage to making such a change is that it becomes easier to use in Kotlin/JS and with a suspend fun main().

In general, you're not supposed to put runBlocking inside another coroutine, so there's really no option to use the normal run functionality if you are doing argument parsing within an existing coroutine setup. You have to do the suggestion in #98 and run outside of the class (which also isn't a terrible thing).

from clikt.

Those are good points. And running outside of the class only really works well if you don't have any subcommands.

It's a bummer that changing main to a suspend fun would force everyone to deal with coroutines even if they weren't using them otherise.

I don't know, do you think that it's worth making that backwards incompatible change?

from clikt.

While I'm open to this idea of a separate SuspendingCliktCommand, I am strongly opposed to forcing all CliktCommand classes to be suspending and opposed to adding a dependency on kotlinx.coroutines for the core clikt module. There are many cases where people will not use any coroutines and it would be a significant amount of bloating, both cognitive and physical, to be forced to have these functions be suspending.

The main reason I got into making command line programs is to be small and minimalistic. I think its important that this library is a bit more strict with regard to keeping things minimal on the core module.

I think a separate SuspendingCliktCommand could be nice as long as it is not forced on anyone. In my opinion, it can just have a suspend fun run and suspend fun main. It doesn't need to create the coroutine scope itself, and by not creating the scope itself that allows the library user to provide whatever scope they want. Providing a custom scope could be important for cases where a SuspendingCliktCommand is embedded into an application and needs to be scoped to a particular part of the structured concurrency.

from clikt.

I am not opposed to having a separate module that provides an AsyncCliktCommand. I am imagining:

Main module:

• Keeps CliktCommand just as it is, no suspending
• Provides SuspendingCliktCommand, with suspend fun run and suspend fun main but does not create its own scope
• No kotlinx.coroutines dependency

"async" module:

• Has dependency on main module and kotlinx.couritines
• Provides AsyncCliktCommand with a suspend fun run but a non suspending fun main, because the scope is created inside main, with runBlocking or something similar.

from clikt.

I think starting with a SuspendingCliktCommand in the main module with suspend wired through from main to run and annotated with an opt-in annotation indicating it's not stable would be a good starting point. This will allow you the option to tear it out if things go poorly! But it also means user's who entirely encapsulate Clikt can still try it.

I'm also interested in how blocking commands and suspending commands compose in various subcommands setups.

An "async" module as proposed doesn't seem useful enough to me, and generally you don't want to be in the business of creating scopes unless your execution naturally maps to a lifecycle. suspend fun main() maps to the process lifecycle. Clikt's main is just a function like any other. Make the caller do a runBlocking if they truly want to synchronously invoke a suspending command.

It's a bummer that changing main to a suspend fun would force everyone to deal with coroutines even if they weren't using them otherise.

I don't know, do you think that it's worth making that backwards incompatible change?

Yeah function coloring really bites here because really all we want to do is propagate the function color through the library. There are probably a few ways to accomplish this with a significant design change in the library, though.

For example, one way to do this would be to split parsing from executing. Instead if calling main, you would invoke something like parse which would return a user type to then be executed.

Want synchronous?

class MyCommand(..) {
  override val runner = {
    println("Hello")
  }
}

fun main(vararg args: String) {
  MyCommand().parse(args).invoke()
}

Want suspend?

class MyCommand(..) {
  override val runner = suspend {
    delay(1.seconds)
    println("Hello")
  }
}
suspend fun main(vararg args: String) {
  MyCommand().parse(args).invoke()
}

Want... Compose?!?

class MyCommand(..) {
  override val runner = @Composable {
    Text("Hello")
  }
}

suspend fun main(vararg args: String) = runMosaic {
  val runner = MyCommand().parse(args)
  setContent(runner)
}

Granted this is very off-the-cuff and shouldn't be taken too seriously verbatim.

from clikt.

Thanks everyone for the feedback so far, it's really helpful.

I agree that splitting out parsing seems like the way to approach this; I've done some experiments with that as a way to solve #342 and #489, but haven't come up with anything I'm happy with yet. I'll take another stab at it though.

from clikt.

Splitting parsing and executing in this way sounds like an excellent idea!

from clikt.

So I thought I'd share the design I'm working on in case anyone had feedback.

I'm introducing a new base class with a generic runner val:

abstract class BaseCliktCommand<RunnerT : Function<*>> {
    abstract val runner: RunnerT
    // everything currently in CliktCommand moves here
}

abstract class CliktCommand: BaseCliktCommand<() -> Unit>() {
    final override val runner: () -> Unit get() = ::run
    abstract fun run()
}

The abstract fun run() isn't strictly necessary, but keeps source compatibility with the current design, and seems a little nicer than making everyone override the val. The Function<*> constraint on the generic also isn't required, I might take it out so that you could return a Flow or something?

I'll probably also remove all of the constructor parameters to CliktCommand and make them open properties instead so that you don't have to forward them all from subclasses. Maybe keep just name as a parameter, since that one gets used a lot?

The generics on subcommands end up a little hairy, but they ensure that all the subcommands have the same runner type:

fun <RunnerT : Function<*>, CommandT : BaseCliktCommand<RunnerT>> CommandT.subcommands(
    vararg commands: BaseCliktCommand<RunnerT>,
)

I'll provide a way to manually control parse and finalize:

object CommandLineParser {
    // does not throw, returns info on which commands to run and a list of any errors encountered
    fun <RunnerT : Function<*>> parse(
        command: BaseCliktCommand<RunnerT>, originalArgv: List<String>,
    ): CommandLineParseResult<RunnerT> 

    // throws exceptions encountered during finalization
    fun finalize(invocation: CommandInvocation<*>)
}

Then the current parse and main methods move to extensions:

fun BaseCliktCommand<() -> Unit>.parse(argv: List<String>) {
    val result = CommandLineParser.parse(this, argv)
    result.throwErrors()
    for (invocation in result.invocations) {
        CommandLineParser.finalize(invocation)
        invocation.command.runner()
    }
}

I'll provide base commands for a couple of different runner types:

abstract class SuspendingCliktCommand: BaseCliktCommand<suspend () -> Unit>() {
    override val runner: suspend () -> Unit get() = ::run
    abstract suspend fun run()
}

and maybe

/** Passes the output of one subcommand to the next one */
abstract class ChainedCliktCommand<T>: BaseCliktCommand<(T) -> T>() {
    override val runner: (T) -> T get() = ::run
    abstract fun run(t: T): T
}

fun <T> BaseCliktCommand<(T) -> T>.parse(argv: List<String>, initial: T): T {
    var value = initial
    val result = CommandLineParser.parse(this, argv)
    result.throwErrors()
    for (invocation in result.invocations) {
        CommandLineParser.finalize(invocation)
        value = invocation.command.runner(value)
    }
    return value
}

Anyway, that's what I'm working on. It's a fair amount of work since I have to rewrite most of the internals and handle all of the parsing edge cases without being able to interleave parsing and finalization. Fortunately, the work done in #474 makes it possible.

from clikt.

Thank you for sharing. Great ideas.

The Function<*> constraint on the generic also isn't required, I might take it out so that you could return a Flow or something?

I vote for not having the Function<*> constraint. Being allowed to produce a Flow or something sounds cool.

One small naming question that arises if we don't include an upper bound of Function<*> is that the name "runner" might because a misnomer, since the parsing result might not be something that is "run". Maybe rename it to parseResult? Though, this doesn't matter too much and I can also see the argument for keeping it as runner.

Overall this update seems like it will be really useful.

from clikt.

The runner isn't the result of parsing (there is a ParseResult class that CommandLineParser.parse returns), it's a user-defined function you can call after parsing is complete.

I'm definitely open to any naming suggestions, though. Now's the time to bikeshed that sort of thing.

from clikt.

Let me see if I understand this correctly.

CommandLineParser.parse will return a ParseResult

ParseResult will hold:

• a BaseCliktCommand, which now should have its parameters filled.
• exceptions, to be thrown by throwErrors
• invocations is a collection of the main command and all subcommands that have been called. With the rule being that they all have the same generic type of runner (or whatever it ends up being called)

I do not know what CommandLineParser.finalize will be for, but it is apparently an intermediate step to run after throwing parsing errors (so it only runs if there are no parsing errors) and right before handling the runner.

And then the runner object from each command can be "handled", where "handled" can mean literally running or, dependending on the generic type of "runner", in some other way.

I think this is just making me wonder why exactly runner and the generic type argument should exist in the library at all for BaseCliktCommand. What if BaseCliktCommand was not generic and simply didn't have a runner object defined, not even an abstract one.

In most circumstances subclasses such as CliktCommand and SuspendingCliktCommand will be used. So it makes sense for them to have a runner object and a run method.

But the only time BaseCliktCommand will be used is for creating custom subclasses. What I'm thinking is that any time that anyone creates a custom subclass of BaseCliktCommand, it seems that they pretty much always will also be writing a custom parse extension function to go along with it. And given that, I'm not quite seeing what purpose it serves for runner to be included in BaseCliktCommand.

Let me try to explain with an example.

Say that this is BaseCliktCommand:

// note it no longer needs a generic parameter
abstract class BaseCliktCommand {
    // everything currently CliktCommand, minus `run` and `runner`
}

And now say I define for myself:

abstract class FlowCliktCommand<out T>: BaseCliktCommand {
    val flow: Flow<T> = // create a special flow based on the parameters
}

Now I will need to also write my parse function:

fun FlowCliktCommand<T>.parse(argv: List<String>): Flow<T> {
    val result = CommandLineParser.parse(this, argv)
    result.throwErrors()
    
    return flow {
        for (invocation in result.invocations) {
            CommandLineParser.finalize(invocation) // maybe this belongs outside the flow?
            emitAll(invocation.command.flow)
        }
    }
}

The only point I am making here is that I created my perfect custom FlowCliktCommand and didn't need any predefined runner property. So maybe excluding runner from the BaseCliktCommand and removing the generic param would simplify things for both you and users?

from clikt.

Sometimes a naming issue hints at a design issue... when it isn't procrastination or bikeshedding

from clikt.

finalize takes the parsed command line, runs all the parameter conversion and validation, and sets the property values.

The generic parameter allows us to enforce that all subcommands have the same runner type as their parent command.

The parse result class looks like this:

data class CommandInvocation<RunnerT>(
    val command: BaseCliktCommand<RunnerT>,
    val optionInvocations: Map<Option, List<Invocation>>,
    val argumentInvocations: List<ArgumentInvocation>,
)

class CommandLineParseResult<RunnerT>(
    val invocations: List<CommandInvocation<RunnerT>>,
    val errors: List<CliktError>,
)

Without the generic parameter, you'd have to cast the BaseCliktCommand down to your expected type and hope no one accidentally set a CliktCommand as the child of your FlowCliktCommand.

It also means that registeredSubcommands returns a List<BaseCliktCommand<RunnerT>>, so the parser internals can iterate over subcommands without any unsafe casts.

I'm seeing some unnecessary repetition, so I'll change CommandLineParser.parse to throw exceptions by default so that folks don't need to (and don't forget to) call result.throwErrors() unless they want to.

from clikt.

The generic parameter allows us to enforce that all subcommands have the same runner type as their parent command.

Thank you for explaining about the type checking here. I agree that we don't want to require downcasting.

Could we have a self-referencing type parameter? I am curious if this could give us the same type checking benefits you described in your last comment, but would still be able to remove the unnecessary runner. Also, subcommands could be type checked according to just the command class itself which could mean even more type checking benefits if we want to use members other than runner inside the parse function, I think?

I'm sharing the messy snippet below just to show that the type checking seems to work all around as expected in various scenarios.

abstract class BaseCliktCommand<C: BaseCliktCommand<C>> {
    // everything currently CliktCommand, minus `run` and `runner`
    private val mutableSubCommands = mutableListOf<C>()
    val subCommands: List<C> = mutableSubCommands
    fun addSubCommand(subCommand: C) = mutableSubCommands.add(subCommand)
}

abstract class NormalCliktCommand: BaseCliktCommand<NormalCliktCommand>() {
    abstract fun run()
}

class MyNormalCliktCommand1: NormalCliktCommand() {
    init {
        addSubCommand(MyNormalCliktCommand2())
    }
    override fun run() {
        println("1")
    }
}
class MyNormalCliktCommand2: NormalCliktCommand() {
    override fun run() {
        println("2")
    }
}

abstract class FlowCliktCommand<T>: BaseCliktCommand<FlowCliktCommand<T>>() {
    val flow: Flow<T> = flow {
        emitOutput()
    }
    protected abstract suspend fun FlowCollector<T>.emitOutput()
}

object CommandLineParser {
    fun <C: BaseCliktCommand<C>> parse(command: BaseCliktCommand<C>,arv: List<String>): CommandLineParseResult<C> {

    }
    fun <C: BaseCliktCommand<C>> finalize(invokation: CommandInvocation<C>) {

    }
}
data class CommandInvocation<C: BaseCliktCommand<C>>(
    val command: C,
    val optionInvocations: Map<String,String>,
    val argumentInvocations: List<String>,
)

class CommandLineParseResult<C: BaseCliktCommand<C>>(
    val invocations: List<CommandInvocation<C>>,
    val errors: List<Exception>,
) {
    fun throwErrors() {
        errors.forEach {
            throw it // in real implementation combine them or whatever
        }
    }
}

fun <T> FlowCliktCommand<T>.parse(argv: List<String>): Flow<T> {
    val result = CommandLineParser.parse(this, argv)
    result.throwErrors()

    return flow {
        for (invocation in result.invocations) {
            CommandLineParser.finalize(invocation) // maybe this belongs outside the flow?
            emitAll(invocation.command.flow)
        }
    }
}

abstract class NumberFlowCommand<N: Number>: FlowCliktCommand<N>()
class AnyNumberFlowCommand: NumberFlowCommand<Number>() {
    override suspend fun FlowCollector<Number>.emitOutput() {
        emit(1)
        emit(2.0)
    }
}
class IntFlowCommand: NumberFlowCommand<Int>() {
    override suspend fun FlowCollector<Int>.emitOutput() {
        emit(1)
        emit(2)
    }
}
fun main() {
    IntFlowCommand().addSubCommand(IntFlowCommand())
    AnyNumberFlowCommand().addSubCommand(AnyNumberFlowCommand())
    IntFlowCommand().addSubCommand(AnyNumberFlowCommand()) // compilation error here, as expected
}

from clikt.