Cli: v2 feature: Make error message optional in BeforeFunc

[creekorful]

Checklist

• [X] Are you running the latest v2 release? The list of releases is here.
• [X] Did you check the manual for your release? The v2 manual is here
• [X] Did you perform a search about this feature? Here's the Github guide about searching.

What problem does this solve?

Displaying the help message in case of error in BeforeFunc should be optional. In my case I am using the BeforeFunc to validate command context and fails if something goes wrong, and I don't want any help message displayed.

Solution description

I have add a HideHelpOnBeforeError flag to allow to hide help message in case error happens in the BeforeFunc.

[lynncyrin]

@creekorful hiya, please get buy-in from a maintainer before you make a PR that changes our API surface! it's important that it's well curated.

[creekorful]

@creekorful hiya, please get buy-in from a maintainer before you make a PR that changes our API surface! it's important that it's well curated.

Hello! What do you mean by buy-in exactly ?

[lynncyrin]

Hello! What do you mean by buy-in exactly ?

You need the agreement of someone who maintains a package before you make a externally visible change to that package

[lynncyrin]

To repeat my comment in https://github.com/urfave/cli/pull/1082, I don't think a HideHelpOnBeforeError config is the right solution here. I would be interested in discussing any other potential solutions to this problem 👍

[creekorful]

Hello back!

A proper solution, for me, would be to let the user decide of the behavior in case of error, instead of printing the error & the help usage.

In the RunAsSubcommand method, the BeforeFunc is processed like this:

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

which is more cleaner way, since we let the user decide what he wants to do with the error.
(by implementing the ExitErrHandler function)

I think we should unify the way BeforeFunc are handled in RunContext & RunAsSubcommand

What do you think?

[rliebz]

@creekorful that makes a whole lot of sense to me. Two features of this strike me as really important:

• The behavior of RunContext and RunAsSubcommand should be as similar as possible.
• Calling ShowAppHelp effectively assumes that any error encountered in Before is a usage error, and that does not strike me as a safe assumption to make.

[creekorful]

• Calling ShowAppHelp effectively assumes that any error encountered in Before is a usage error, and that does not strike me as a safe assumption to make.

That's a really good point.

What do you think about about making a PR to unify the behavior as in RunAsSubcommand ?

[rliebz]

Curious what @lynncyrin thinks, but I'd be :+1: for that solution.

I'd say feel free to open a PR whenever. For me, the big reason to get buy-in before making a change to the API surface is that spending the effort to curate and submit a code change, only to find out that it wasn't something that the maintainers were interested in the first place, is a not-fun situation on both sides. If the change is only as big as deleting a couple lines of code, ¯\_(ツ)_/¯

[creekorful]

Hello there, PR is done.

Thank you

[lynncyrin]

Nice work! ✨