Make Value a drop-in replacement for flag.Value

[tomasaschan]

Fixes #228.

This should allow users to provide their own types implementing flag.Value also to pflag, as pflag.Value is now equivalent to the standard-library variant.

The Type() method is moved to a wrapper interface TypedValue and code relying on it must first test that the provided value implements this interface; this has been done in all code in pflag and tests are still passing, so I'm fairly confident that the changes are correct even if I haven't actually tried them out on a use case with a user-defined Value implementation (without Type()).

[thaJeztah]

This is a tricky one; this change removes the Type() string from the interface, which means that if someone used this interface for other code, and expect values to have a Type() function, they may now end up with a value that doesn't support it.

IIUC, the change is meant to make the Type() optional for functions in this repository, so that they can accept both stdlib Values and spf13/pflag values (which are "richer" as they provide the Type() method).

An alternative approach would be to;

• Add a new StdValue interface, which is the reduced interface
• Keep the Value interface untouched, but (optionally) alias it to TypedValue (mostly to be more expressive in its intent)

However, looking at stdlib; it looks like stdlib defines a flag.Value interface (which was already available in go1.12, so safe to use: https://pkg.go.dev/[email protected]#Value); perhaps it's better and more transparent to not define a local interface for that, but to embed the one from stdlib.

• Update all signatures that currently require a spf13/pflag.Value to use flag.Value (and be more permissive); at a quick glance, none of the FlagSet methods are part of an interface, so possibly they can be made more permissive. Either that, or new methods to be added (but maybe that's not needed).
• Add the new TypedValue interface, and make it embed the stdlib flag.Value
• Deprecate the old spf13/pflag.Value interface in favor of TypedValue or flag.Value ("depending on the use-case")

So something like;

// Value is the interface to the dynamic value stored in a flag as implemented.
//
// Deprecated: use [flag.Value] or [TypedValue], depending on your use-case.
type Value = TypedValue

// TypedValue wraps extends [flag.Value] with an additional Type() string, which
// can be used to special-case things like usage instructions, error
// messages, parsing, etc.
// Its value should be a constant string that identifies the type of
// the underlying flag value.
type TypedValue interface {
	flag.Value
	Type() string
}

[thaJeztah]

Hm... overlooked this part; this will be tricky, because we're removing the Type() from the Flag.Value - that will likely break people as they would now have to implement code similar to what's in the PR to assert that the flag Value implements the TypedValue;

pflag/flag.go

Lines 192 to 196 in 1043857

	type Flag struct {
	Name string // name as it appears on command line
	Shorthand string // one-letter abbreviated flag
	Usage string // help message
	Value Value // value as set

[thaJeztah]

Perhaps this requires a utility to convert/wrap as StdLib value to TypedValue (assign a type), but keeping this Field to implement Type() (TypedValue to be explicit).

[tomasaschan]

I guess the (philosophical) question is whether the breaking change is introducing this method on the Value type (thus breaking the promise of being a drop-in replacement for stdlib flag), or removing it. If one takes the former stance, then this is a bug fix; with the latter, it's a major version bump. (So after all the flak for 1.0.8, it turns out it's not even clear cut what a breaking change is. Who'da thunk? 😉)

My guess is that there are quite a few instances of people implementing Type because our interface requires it from them, but much fewer (if anyone) actually using the method for anything on their end.