Need proper documentation on how to correctly handle errors

[foogod]

(This is related to #306 and #361, I think, but arguably a separate issue)

It is extremely unclear how to correctly handle error return codes from the Parse functions. Most of the examples just use panic(err), but even in the most basic case even that is arguably wrong/broken, because it results in printing the error message twice in most cases (because the library itself prints the error before returning in many situations).

However, I just recently discovered that just doing an os.Exit(1) (without printing the error) is also not correct in all cases, as there are also some cases where the library does not print the error (for example, a malformed structure annotation) and therefore if you do not actually print the error returned in those cases, the program just exits silently instead.

However, as far as I can tell there is no documentation anywhere on how to figure out what the correct action to take is (print the error, don't print the error, do something else, etc) when any particular error value is returned. This is all just a big mess.

Error handling is one of the most basic functions needed when parsing user input, and I simply can't figure out how to get this library to do it correctly in all cases. I think I'm going to have to look for some other library to use instead..

[oubiwann]

Yeah, + 💯 to everything you've said, @foogod -- I finally needed to sit down and get this sorted for work. Over the past few months, I've probably accumulated about a day or two of wasted time on this. Will be leaving extensive code comments for current/future SWEs on our team ...

Having suffered so much at the hands of other Go flags/getopts/CLI tools, I'm still sticking with this one, though. Most of the decisions about which I care most deeply were designed "correctly" in this lib.

[oubiwann]

I fixed this for work, and since it was generally useful, I pushed it up here, too:

• https://github.com/geomyidia/flagswrap

This should bubble everything up to the user and provide a consistent way of checking for errors using this library ...

[borud]

It would, at the very least, be helpful if the project had a cut and paste'able example of how to correctly deal with errors.

But a fix would be preferable.

[illarion]

I would propose just to change default example to something like this:

opts := &Opts{}
_, err := flags.Parse(opts)
if err != nil {
	if w, ok := err.(*flags.Error); ok {
		if w.Type == flags.ErrHelp {
			os.Exit(0)
		}
	}
	os.Exit(1)
}

[borud]

@illarion The os.Exit(1) should print an error message. In its current form it will swallow any errors that stem from invalid tags (for instance if you have a duplicated flag).

[illarion]

@borud flags prins error message by itself, to the stderr, so that this code does exactly the same thing as the original example intended to do, but more correctly.

[foogod]

@illarion please see my note in the original report that there are also some cases where the library actually does not print an error message as well, in which cases os.Exit(1) is actually very bad behavior, because it just exits silently with no explanation of what's going on. This was actually the biggest cause of my original frustration, and is still not really addressed by your updated version..

[illarion]

@foogod oh, ok, that makes sense. So, it seems that there is no possible way to use go-flags properly then? It's a good case for a pool request then. Please correct me if I'm wrong.

[foogod]

To be honest, I'm not familiar enough with the code (and the intended behavior) to know whether there is or not, which is sorta why I posed this as a documentation problem, but it may also be a more fundamental functionality issue, I'm not sure.

I don't know, for example, whether there is some way to just tell by looking at the error value whether it is something that has already been printed (and so we should just os.Exit) or whether it is a type which is not printed by default (so we need to print our own message first).. If that is possible, then just documenting how to do that correctly might be enough (though arguably, I think it would be a better interface if the caller didn't have to do that sort of thing themselves, so it might arguably be better to change the code to just make sure that it's always one way or the other anyway)..

[borud]

No, there is actually no good way to use go-flags properly which is why I have started using it less and less. Fixing this would actually be more useful than risk breaking any past behaviors. Does anyone know if the owner accepts pull requests?

[eddsalkield]

How about the following solution?:

var opts Options
parser := flags.NewParser(&opts, flags.Default & ^flags.PrintErrors)
_, err := parser.ParseArgs(os.Args)
if err != nil {
    fmt.Fprint(os.Stderr, err)
    os.Exit(1)
}

This will always exit with code 1, and will only print the errors once by disabling the default error printing.

[suhlig]

Thanks @eddsalkield and @illarion, I ended up using a combination:

if err != nil {
	if e, ok := err.(*flags.Error); ok {
		if e.Type == flags.ErrHelp {
			fmt.Println(err)
			os.Exit(0)
		}
	}
	logger.Fatal(err)
}

log.Fatal(err) prints the error and then exits non-zero.

[eddsalkield]

Sounds good :)
Fwiw, I ended up dropping the dependency on this package and switching to flag as in the stdlib: https://pkg.go.dev/flag