Inconsistent error handling between App.OnUsageError and App.Before during App.Run

[diliop]

When OnUsageError is set for a cli.App instance, one has full control of what is printed to stderr when an error is encountered. That is not the case when setting Before.

Looking at how OnUsageError is handled in App.Run in app.go [c6eb2a0]:

if a.OnUsageError != nil {
	err := a.OnUsageError(context, err, false)
	a.handleExitCoder(context, err)
	return err
}

and for Before:

if a.Before != nil {
	beforeErr := a.Before(context)
	if beforeErr != nil {
		fmt.Fprintf(a.Writer, "%v\n\n", beforeErr)
		ShowAppHelp(context)
		a.handleExitCoder(context, beforeErr)
		err = beforeErr
		return err
	}
}

The issue I'm having is with the call to ShowAppHelp(..) when Before is set. Essentially the experience to the user is different when a usage error is encountered and I want to print custom output versus when I am checking for lets say flag value validity before running any subcommands. Ideally I would prefer the 2 if statements above to be identical and the default case to call ShowAppHelper(..) as is currently the case i.e. for Before:

if a.Before != nil {
	beforeErr := a.Before(context)
	if beforeErr != nil {
		a.handleExitCoder(context, beforeErr)
		err = beforeErr
		return err
	}
}

I've looked around to see if there is another way to modify this behavior but nothing obvious stands out.

[jszwedko]

Hi @diliop,

Thanks for the detailed issue! If I had to summarize it, would it be fair to say that you want to consider errors in Before functions to be considered and handled in the same way as "usage errors"?

The difference when running a subcommand is also confusing.

I agree that these three cases should be unified.

[diliop]

Yes @jszwedko that is correct!

From a philosophical standpoint I want the error handling (and in this case error printing to stdout/stderr) to be the same across all types of errors that arise at different points in the execution of a command/subcommand. This essentially would be the user experience consistent. Currently I can set "usage errors" to print something like this:

cmd subcmd: --flag must be set
See 'cmd subcmd --help'.

which is pretty familiar if one considers popular command line tools i.e. docker, vault etc. I can't though do that for "Before" or "After"-type errors.

[stale[bot]]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[coilysiren]

@asahasrabuddhe I'm un-assigning this since it's been sitting for a while, feel free to re-assign it to yourself if you stand up a PR for it

[stale[bot]]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

[stale[bot]]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[rliebz]

Bump.

Related: https://github.com/urfave/cli/issues/1081

[stale[bot]]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

[stale[bot]]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[stale[bot]]

Closing this as it has become stale.