Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify flag.Usage implementation. #12

Merged
merged 1 commit into from Jul 11, 2016
Merged

Simplify flag.Usage implementation. #12

merged 1 commit into from Jul 11, 2016

Conversation

dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Jul 9, 2016

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 mdempsky/unconvert@3b9aa41 and shurcooL-legacy/backlog#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 馃槈).

Remove unnecessary os.Exit call. The flag package is responsible for doing that.
@campoy campoy merged commit 1df02eb into campoy:master Jul 11, 2016
@campoy
Copy link
Owner

campoy commented Jul 11, 2016

Nice, thanks! I was not aware of PrintDefaults calling os.Exit 馃槃

@dmitshur dmitshur deleted the simplify-flag-Usage branch July 12, 2016 17:19
@dmitshur
Copy link
Contributor Author

dmitshur commented Jul 16, 2016

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.

dmitshur added a commit to shurcooL/ivybrowser that referenced this pull request May 31, 2017
usage is made simpler and the behavior of ivy remains unchanged. This
is because the flag package already calls os.Exit(2) inside flag.Parse
when the parse fails.

That is what the flag package API is. It may be unfortunate, but it
cannot be changed in Go 1.

See mdempsky/unconvert@3b9aa41,
campoy/embedmd#12, and https://upspin-review.googlesource.com/c/9642/
for precedent.

Reference: https://dmitri.shuralyov.com/idiomatic-go#don-t-os-exit-2-inside-flag-usage.

Fixes the following issue:

	$ staticcheck github.com/shurcooL/ivybrowser
	ivy.go:46:7: the function assigned to Usage shouldn't call os.Exit, but it does (SA1022)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants