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

proposal: flag: add Strings, a default Value #40155

Open
carnott-snap opened this issue Jul 10, 2020 · 4 comments
Open

proposal: flag: add Strings, a default Value #40155

carnott-snap opened this issue Jul 10, 2020 · 4 comments
Labels
Projects
Milestone

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Jul 10, 2020

proposal

While there are certainly many custom flag.Value implementations that are required, most of the time, all I want is a []string. As such, I suggest we add a flag.Strings symbol:

package flag

var _ Getter = Strings{}

type Strings []string

func (s Strings) Get() interface{} { return s }
func (s *Strings) Set(v string) error { *s = append(*s, v); return nil }
func (Strings) String() string { return "" }

costs

It is another symbol on the flag package, so it could get lost or be confusing, however I think this will serve to improve customer code considering how much this is implemented, like sort.StringSlice. My biggest concern is if everybody needs a different implementation.

alternatives

If, as @cespare calls out, we are concerned about this custom type proliferating, we can instead expose a constructor: (see below)

func Strings(p *[]string) flag.Value

Unfortunately, this flags.Strings does not work the same as flag.String and simply mirroring that interface seems better:

func Strings(name string, value string, usage string) *[]string
func StringsVar(p *[]string, name string, value string, usage string)

extensions

Because it is easier, I have assumed that all flag.Strings will have the default of emptiness. If it is preferred, we could instead return the first element or a csv joined form of all elements:

func (s Strings) String() string { if len(s) == 0 { return "" } return s[0] }
// or
func (s Strings) String() string { return strings.Join(s, ",") }

If the use case can be justified, it may be worth adding some state and complexity to Strings, but without a clear litmus, it is hard to know where to stop:

type Strings struct{
	Data  []string
	Check func(string) (string, error)
}

func (s *Strings) Set(v string) error {
	if *s.Check != nil {
		var err error
		if v, err = s.Check(v); err != nil {
			return err
		}
	}
	*s.Data = append(*s.Data, v)
	return nil
}

That being said, there are real flexibility benefits to hiding the implementation in a struct:

type Strings struct{ v []string }

func (s Strings) Slice() []string { return *s.v }
func (s *Strings) Set(v string) error { *s.v = append(*s.v, v); return nil }
@gopherbot gopherbot added this to the Proposal milestone Jul 10, 2020
@gopherbot gopherbot added the Proposal label Jul 10, 2020
@cespare
Copy link
Contributor

@cespare cespare commented Jul 10, 2020

As it happens, in 2015 we added exactly this API (same names and everything) to our internal library at my company. It got a lot of use and it works well enough. During those five years, though, we found that there is a downside: this Strings type has a way of leaking out into the code, and types/functions that only care about a []string end up referring to this flag helper type and needing to call s.Slice() and so on.

The reason this happens is because of code like this:

type conf {
	things flagext.Strings
	// ... and lots more
}

func main() {
	var c conf
	flag.Var(&c.things, "thing", "Specify things (may be given multiple times)")
	flag.Parse()

	// ...
}

Of course you can always avoid this by declaring the Strings value separately:

type conf {
	things []string
	// ... and lots more
}

func main() {
	var c conf
	var things flagext.String
	flag.Var(&things, "thing", "Specify things (may be given multiple times)")
	flag.Parse()

	c.things = things

	// ...
}

but in practice, people usually end up doing the former because it's shorter and more direct.

A month ago I changed our API to the following:

// Strings creates a flag.Value for a []string where providing the flag multiple
// times appends to the slice. The pointer p must be non-nil and points to a
// slice containing the default value for the flag. If the user provides the
// flag, it overrides the prepopulated slice.
func Strings(p *[]string) flag.Value

(Essentially, this is taking the flag.VarX approach instead of the flag.X approach.)

This is a smaller API and lets us do what we want more directly, which is to have the flag directly manipulate a []string.

type conf {
	things []string
	// ... and lots more
}

func main() {
	var c conf
	flag.Var(flagext.Strings(&things), "thing", "Specify things (may be given multiple times)")
	flag.Parse()

	// ...
}
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jul 10, 2020

While it may not be as intuitive to beginners, types have call site usage that feels like a pseudo function. As such, and I think the utility of the exported type can justify this otherwise arcane syntax: (though it would be nice if you could &flag.Strings(c.things))

var c conf
flag.Var((*flag.Strings)(&c.things), "thing", "Specify things (may be given multiple times)")
flag.Parse()

I am also unclear how your flagext.Strings plays with the style of the library. If we decide the type is not what is needed, I think these two functions would be more intuitive:

package flag

func Strings(name string, value string, usage string) *[]string
func StringsVar(p *[]string, name string, value string, usage string)
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Aug 6, 2020

@ianlancetaylor: can you add this to the proposals project?

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 6, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 6, 2020

@carnott-snap Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.