proposal: flag: add Func

[earthboundkid]

I wrote a library of extensions to the flag package. Most of them are too specific to fit into flag itself or easy enough to write on one's own that there's no real point in adding them to the standard library, but there is one extension that I think would make a good addition to the standard library, flagext.Callback.

flagext.Callback is essentially just a wrapper that converts a function to a flag.Value. flag.Value has two methods (plus an optional third method for flag.Getter), but the String() string (and Get() interface{}) method is pure boilerplate and can be ignored by just putting a Stringer onto a bare func(s string) error.

My function's signature is

// Callback is a convenience function for defining a Set(string) error function
// on a flag.FlagSet without constructing a new type.
//
// If nil, fl defaults to flag.CommandLine. Value is only used for showing the
// default in usage help.
func Callback(fl *flag.FlagSet, name, value, usage string, cb func(string) error)

Example:

mode := "none"
flagext.Callback(fs, "mode", mode, "what mode to use", func(s string) error {
    if s != strings.ToLower(s) {
        return fmt.Errorf("mode must be lower case")
    }
    mode = s
    return nil
})

I think that this, like http.HandlerFunc, is so convenient that it would be good to have in the standard library. If it were in the standard library, it could be a method on flagset instead of a function that takes a flagset, so adding it to the standard library would have advantages that couldn't be reproduced by just importing my external package.

My proposed addition is

func (f *FlagSet) Callback(name, value, usage string, cb func(string) error)

and the corresponding top level package function.

[rsc]

In the compiler we have this badly-named function:

func Flagfn1(name, usage string, f func(string)) {
	flag.Var(fn1(f), name, usage)
}

type fn1 func(string)

func (f fn1) Set(s string) error {
	f(s)
	return nil
}

func (f fn1) String() string { return "" }

A better name would be flag.Func probably.

One downside of this is that returning the empty string from String means that flag.PrintDefaults never prints a default for that setting. Maybe that's OK.

It's unclear that the value argument is necessary in this case. Maybe the API (at the top level, same in FlagSet) would be:

func Func(name, usage string, f func(string))

Thoughts?

/cc @robpike

[earthboundkid]

In my flagext package, I set the default in Usage with value but have to explain in the docs that it doesn't do anything besides change the help text. It might be simpler to just drop it and let users write (default ...) by hand in their usage string.

[earthboundkid]

I think it should be func(string) error, not just func(string). Returning an error lets you complain if the value is malformed or unusable.

[earthboundkid]

Sorry for the rapid fire responses.

If the signature were func Func(name, usage string, f func(string) error) and you wanted to just use func(string), it's very easy to add an inline wrapper that returns nil, but vice-versa, it's not clear what to do with an error if you have func(string) error and need func(string).

[rsc]

Yes, returning an error from the func makes a lot of sense.
(In the compiler I bet the func just calls fatal, but that's not right in general.)

OK, so it sounds like the API is:

func Func(name, usage string, f func(string) error)

Any other thoughts?

[earthboundkid]

Seems good to me. :-) What should the example in the docs do that would be wryly funny?

[gopherbot]

Change https://golang.org/cl/240014 mentions this issue: flag: add Func

[robpike]

There's a CL but the proposal is still open. I had to read the issue and the CL code several times to understand what this is and why we want it. It's not a bad idea, just badly explained.

[earthboundkid]

Yes, the code part of the CL is trivial, so I just sent it off during a down period today. The hard part will be writing a good docstring.

[rsc]

Ignoring the CL, it sounds like people are generally in favor of the API discussed above.

Based on the discussion, then, this seems like a likely accept.

[rsc]

We can work out the docs separately. No change in consensus, so accepted.

[earthboundkid]

Now that Go 1.15 is out, can the CL be reviewed or do I need to do something else first?

[dolmen]

For those stuck with Go below 1.16, you can already use the type Func from package github.com/dolmen-go/flagx with flag.Var.