Flag errors are not `ExitCoder`

[Nokel81]

my urfave/cli version is

v2.2.0

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 problem? Here's the Github guide about searching.

Dependency Management

• [x] My project is using go modules.
• [x] My project is using vendoring.
• [ ] My project is automatically downloading the latest version.
• [ ] I am unsure of what my dependency management setup is.

Describe the bug

If you mark a flag as "required" for instance and it is missing, then the error that is bubbled up to the root cli.App.Run(..) call does not satisfy the required interfaces for HandleExitCoder.

To reproduce

1. Have a required flag
2. Have a panic after the call to HandleExitCoder
3. Call the CLI without it

Observed behavior

The panic is called

Expected behavior

HandleExitCoder calls os.Exit(..)

Additional context

It should be possible to define the exit code for a specific flag (or for the application in general) if there are flag errors. Be them required, or not defined as a flag.

[coilysiren]

Makes sense! I would be happy to review a PR from anyone that fixes this 🙏

[Nokel81]

Would you prefer it to be a global setting for the whole App or Command or one specific to a flag?

[coilysiren]

I'm not quite sure I'm understanding where a setting is coming into play here? I think my expectation is that the existing required flags error satisfies that HandleExitCoder interface.

[Nokel81]

Yes but what should that exit code be? Should it be static and unchanging? I think that it should be able to be set by users of this library.

[coilysiren]

Ah, gotcha! I'm of the opinion that we should either:

• exit 0 with a global setting for configuring that
• return when hitting required flags errors, in which a way that an explicit os.Exit isn't required

I'd be interested in more opinions from @urfave/cli though!

[Nokel81]

I think returning an ExitCoder should be part of it.

Another option (I like this one because it is the most customization while still having a fast path for global setting):

• Have a hierarchy of exit codes: use app setting or, if set, command setting or, if set, flag setting

[Nokel81]

Though, now that I have looked more deeply at the code, there is a slight problem with having the flags define their own exit codes. What happens if multiple flags produce errors (currently only required).

• Should the first win?
• Should the last win?
• Should the highest value win?
• Should the lowest non-zero win? (What about negative values?)

I have looked at some what other programs do (such as posix exit). It seems to be undefined if you try and use a negative number, or if you try and use a number outside of [0, 255] since that is all that wait_pid will return. All this is to say that the last option (lowest wins) should probably be never considered.

[coilysiren]

Nice investigative work ✨

[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.

[Nokel81]

bump

[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.

[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.

[dearchap]

@urfave/admins Any thoughts on this ?