Subcommands should have first-class support for errors

[Carrotman42]

Currently, the CommandRun.Run method returns an int. I suspect this is because the original design wanted commands to decide their exit code, but it causes some unfortunate boilerplate: most commands will end up with an error value, and have to manually print it out and decide some code to return, adding a few lines on every return statement.

Some people mitigate some awkwardness by introducing a helper function to call, which translates an error into an int (and prints non-nil errors), but that has other awkwardness in that you have to remember to use that in every return statement in every command's top-level Run method.

Instead of a helper function that you have to call, I recently introduced a wrapper type in my project which allows commands to implement a Run method which returns an error, and then does the error check and translation to integer code in one place. (It also fixes the lack of context.) It works for my project, but ideally this convenience would be pushed into this library too for everyone to share.

I would suggest instead to have the Run method return an error instead of an int; I suspect most commands would be content with exiting with 0 on a nil error, and a 1 on an arbitrary error. For those (few) commands which want to control their exit code, we can introduce an extra type like:

type ExitCode struct {
   Err error
   Code int
}

Directly changing the CommandRun function would be a breaking change, and I'm not sure how amenable people are to that. (With Go modules, it should be slightly easier to avoid breaking people? I think...) But an obvious second choice would be to deprecate the old type and make a new one which returns an error (and, ideally, takes a context too to solve https://github.com/maruel/subcommands/issues/4).

[maruel]

Hi!

subcommands predates go 1.7 so it predates context.Context. In the original design I wanted to make it possible to run tests in parallel thus the GetOut() and GetErr() but in practice I feel this mostly failed. I wanted the package to be as minimalistic as possible, most CLI packages either reimplement the package flag themselves or just have a huge API surface.

It's been a while I would like to create a v2 that would address the concerns you raised. Would you like to design one?

[Carrotman42]

Thanks for the response!

I don't have that much experience with this library, but I have recently joined a team full-time which is using it so I will quickly gain some experience.

I am happy to keep in mind what a v2 API would look like as I learn more, and will happily tap out a proposal once I think I have a good enough handle on things. I'll post back any progress on this bug. Thanks for the opportunity!