campoy / embedmd

embedmd: embed code into markdown and keep everything in sync
Apache License 2.0
767 stars 62 forks source link

Simplify flag.Usage implementation. #12

Closed dmitshur closed 8 years ago

dmitshur commented 8 years ago

Remove unnecessary os.Exit call. The flag package is responsible for doing that.

You can look at the default implementation of flag.Usage and confirm it doesn't call os.Exit(2) either:

var Usage = func() {
    fmt.Fprintf(os.Stderr, "Usage of %s:\n", os.Args[0])
    PrintDefaults()
}

Also see https://github.com/mdempsky/unconvert/commit/3b9aa41c71a855a5f5fee96dc751979ce6e8cbbe and https://github.com/shurcooL/backlog/issues/4.

This is a really minor thing, but many people will be looking at this code, so I'm happy to help contribute improvements to style so that other people have a better example to learn/copy from (plus, I don't wanna have to keep making these PRs on even more repos 😉).

campoy commented 8 years ago

Nice, thanks! I was not aware of PrintDefaults calling os.Exit :smile:

dmitshur commented 8 years ago

For posterity and to be more precise, PrintDefaults doesn't call os.Exit, what happens is internal code in flag package in (f *FlagSet) parseOne() returns a non-nil error when it finds a flag that's not registered (and isn't "help"):

return false, f.failf("flag provided but not defined: -%s", name)

Then that error gets handled in flag.Parse depending on errorHandling, which for default flag set happens to be ExitOnError, and that triggers the case which does os.Exit(2). :)

See https://godoc.org/flag#ErrorHandling and https://godoc.org/flag#CommandLine.

So another custom flag set could perform panic on error or even continue on error behaviors instead.