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 observer for child commands run call #434

Closed
cmorigaki opened this issue Jul 31, 2023 · 2 comments · Fixed by #435 or #508
Closed

Support observer for child commands run call #434

cmorigaki opened this issue Jul 31, 2023 · 2 comments · Fixed by #435 or #508

Comments

@cmorigaki
Copy link

Hi, I'm adding analytics to the CLI and one of the use cases is sending an event of the invoked command with its arguments/options. To avoid repetition and ensure every command sends the event, my current implementation looks like this:

abstract BaseCommand(...) : ) : CliktCommand(...) {
    final override fun run() {
        sendEvent()
        this.onRun()
    }
    abstract fun onRun()
}

Two Clikt modifications would improve my code and I believe it would also improve Clikt code:

  1. Add observer for child command run.
    That way instead of creating a Base class, the parent command could observe and send the event after the parsing phase. Also, I would get rid of the final and onRun() workaround because there is no way to force the super method call and ensure the event is sent.

  2. Implement OptionWithValuesImpl.toString() and ProcessedArgumentImpl.toString().
    To collect options and arguments information, I replicated part of the ClickCommand.toString(). Implementing these overrides, would also simplify ClickCommand.toString().

I checked the code and the first one seems not trivial.
For the second, not sure if I'm missing something else here but I would like to contribute with a PR.

@ajalt
Copy link
Owner

ajalt commented Aug 1, 2023

Thanks for the feedback!

The string implementations were all in CliktCommand since, prior to 4.0, there were multiple implementations of Option and Argument. Now that there's only one of each, implementing toString in them makes sense.

I'm not sure about adding lifecycle callbacks. Since it's only a couple of lines of code to do yourself, I'd need to see more use cases before I add a feature like that.

@ajalt ajalt closed this as completed in #435 Aug 1, 2023
@cmorigaki
Copy link
Author

Awesome! Thanks!

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