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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: flag: document behaviour of redefined flags #31694

Open
bontibon opened this issue Apr 26, 2019 · 2 comments

Comments

@bontibon
Copy link
Contributor

commented Apr 26, 2019

Defining a flag with a name that has been previously defined results in the following:

package main

import (
	"flag"
)

func main() {
	fs := flag.NewFlagSet("sample", flag.ExitOnError)

	_ = fs.String("name", "", "name value")
	_ = fs.String("name", "", "name value")

	fs.Parse([]string{`--name`, `test`})
}
sample flag redefined: name
panic: sample flag redefined: name

goroutine 1 [running]:
flag.(*FlagSet).Var(0x430180, 0x147130, 0x40c158, 0x115596, 0x4, 0x11628a, 0xa, 0x7a61)
	/usr/local/go/src/flag/flag.go:850 +0x520
flag.(*FlagSet).StringVar(...)
	/usr/local/go/src/flag/flag.go:753
flag.(*FlagSet).String(0x430180, 0x115596, 0x4, 0x0, 0x0, 0x11628a, 0xa, 0x7a61, 0x40c150, 0x7a61)
	/usr/local/go/src/flag/flag.go:766 +0xc0
main.main()
	/tmp/sandbox904772544/main.go:11 +0xc0

This is OK, but the documentation does not seem to mention that flag names need to be unique for each FlagSet, nor that a panic will occur when trying to create a flag with an already-defined name.

@bontibon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

I also just noticed that f.Output() is written to before the panic call.

fmt.Fprintln(f.Output(), msg)
panic(msg) // Happens only if flags are declared with identical name

This seems like odd be behaviour to me, so perhaps a source comment could be added as to why this is (even better might be to remove the Fprintln, but someone else would have to make that decision (@robpike)).

@bcmills

This comment has been minimized.

Copy link
Member

commented May 28, 2019

the documentation does not seem to mention that flag names need to be unique for each FlagSet

That should probably be fixed, but note that the existence of the Lookup and Set methods already imply that names must be unique.

nor that a panic will occur when trying to create a flag with an already-defined name.

I think that would over-specify the behavior: it is a programmer error to attempt to register a duplicate flag, so the flag package should not guarantee any particular failure mode if that should occur. (For example, it could just as reasonably log an error message and invoke os.Exit.)

@bcmills bcmills added this to the Unplanned milestone May 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.