Skip to content

Make Value a drop-in replacement for flag.Value #436

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.

Sign up for GitHub

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

Open
tomasaschan wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make Value a drop-in replacement for flag.Value #436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions flag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,15 @@ type Flag struct {
type Value interface {
String() string
Set(string) error
}

// TypedValue wraps Value but adds 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 {
Value
Copy link

@thaJeztah thaJeztah Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Type() string
}

Expand Down Expand Up @@ -398,8 +407,8 @@ func (f *FlagSet) getFlagType(name string, ftype string, convFunc func(sval stri
return nil, err
}

if flag.Value.Type() != ftype {
err := fmt.Errorf("trying to get %s value of flag of type %s", ftype, flag.Value.Type())
if tvalue, ok := flag.Value.(TypedValue); ok && tvalue.Type() != ftype {
err := fmt.Errorf("trying to get %s value of flag of type %s", ftype, tvalue.Type())
return nil, err
}

Expand Down Expand Up @@ -597,7 +606,10 @@ func UnquoteUsage(flag *Flag) (name string, usage string) {
}
}

name = flag.Value.Type()
name = "value"
if tvalue, ok := flag.Value.(TypedValue); ok {
name = tvalue.Type()
}
switch name {
case "bool", "boolfunc":
name = ""
Expand Down Expand Up @@ -716,8 +728,14 @@ func (f *FlagSet) FlagUsagesWrapped(cols int) string {
if varname != "" {
line += " " + varname
}
tvalue, tvalueOk := flag.Value.(TypedValue)
if flag.NoOptDefVal != "" {
switch flag.Value.Type() {
if !tvalueOk {
line += fmt.Sprintf(" [=%s]", flag.NoOptDefVal)
return
}

switch tvalue.Type() {
case "string":
line += fmt.Sprintf("[=\"%s\"]", flag.NoOptDefVal)
case "bool", "boolfunc":
Expand All @@ -742,7 +760,7 @@ func (f *FlagSet) FlagUsagesWrapped(cols int) string {

line += usage
if !flag.defaultIsZeroValue() {
if flag.Value.Type() == "string" {
Copy link

@thaJeztah thaJeztah Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@thaJeztah thaJeztah Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

@tomasaschan tomasaschan Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if tvalueOk && tvalue.Type() == "string" {
line += fmt.Sprintf(" (default %q)", flag.DefValue)
} else {
line += fmt.Sprintf(" (default %s)", flag.DefValue)
Expand Down
4 changes: 2 additions & 2 deletions text.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ func (f *FlagSet) GetText(name string, out encoding.TextUnmarshaler) error {
if flag == nil {
return fmt.Errorf("flag accessed but not defined: %s", name)
}
if flag.Value.Type() != reflect.TypeOf(out).Name() {
return fmt.Errorf("trying to get %s value of flag of type %s", reflect.TypeOf(out).Name(), flag.Value.Type())
if tvalue, ok := flag.Value.(TypedValue); ok && tvalue.Type() != reflect.TypeOf(out).Name() {
return fmt.Errorf("trying to get %s value of flag of type %s", reflect.TypeOf(out).Name(), tvalue.Type())
}
return out.UnmarshalText([]byte(flag.Value.String()))
}
Expand Down
4 changes: 2 additions & 2 deletions time.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ func (f *FlagSet) GetTime(name string) (time.Time, error) {
return time.Time{}, err
}

if flag.Value.Type() != "time" {
err := fmt.Errorf("trying to get %s value of flag of type %s", "time", flag.Value.Type())
if tvalue, ok := flag.Value.(TypedValue); ok && tvalue.Type() != "time" {
err := fmt.Errorf("trying to get %s value of flag of type %s", "time", tvalue.Type())
return time.Time{}, err
}

Expand Down
Loading