Make action functions explicitly require the ExitCoder interface instead of an error

[kklipsch]

The current release handles errors returned by the actions/subcommands, the current master branch ignores them. This behavior has flipped back and forth, but the current practice seems like the cli library will ignore errors that are returned by functions passed into it unless they match the ExitCoder interface.

If this is the way forward, make the functions check this at compile time instead of run time.

It seems completely unidiomatic for users of the api to expect errors they return from those functions to be silently ignored, so don't let them assume that they can pass errors back.

(As an aside, I think the whole ExitCoder thing should definitely be opt in and you should leave the behavior of returning errors and them being handled by the library as it is in the current release. But if you are going to go away from that behavior you should enforce it at compile time)

[kklipsch]

Should probably include something like this in the library as well

func WrapExitCoder(f func(*cli.Context) error) func(*cli.Context) cli.ExitCoder {
	return func(cliCtx *cli.Context) cli.ExitCoder {
		err := f(cliCtx)
		if err != nil {
			return cli.NewExitError(err, 1)
		}

		return nil
	}
}

[jszwedko]

Hi @kklipsch,

Thank you far raising this up!

As I mention in #594 (comment); the error isn't silently ignored, but rather it is propagated up to the App.Run() method so that the caller can decide what do with it (RunAndExitOnError being a convenience wrapper for exiting if a non-ExitCoder error occurred). The ExitCoder interface was added to support functions returning the exit code to exit with closer to where the error occurred, but I can see an argument for still not automatically exiting and allowing the caller to assert on the type and decide how to handle it.

I'll mark this as something to reconsider as part of the v2 branch where we can make breaking changes.

[coilysiren]

👋 v2 just hit master, so we'll need to check if this is still a valid issue 🔍

[Nokel81]

This is still a valid issue.

[coilysiren]

@Nokel81 how related is this to #1088?

[Nokel81]

It is only tangentially related. Because v2 has already been released doing this should be part of a v3 since it is a breaking change.

It is related because it is part of the work required in moving more of urfave/cli errors to be ExitCoder

[meatballhat]

(I think this has been said by others in other contexts, but for the record:)

I'm generally against requiring a library-specific error type given the interface-checking behavior that's already available. Please holler if you feel strongly about the intention to move towards a v3 cli.ActionFunc that looks like this:

type ActionFunc func(context.Context, *Command) error

💖 🤘🏼