Repository: urfave/cli
Pull request #1508 (2022-09-30 13:11:10)

Call FlagStringer in String() method of slice flags
## What type of PR is this?

- bug/cleanup

## What this PR does / why we need it:

The default help template relies on the `String()` method of Flag to render the flag. For most flag types, `String()` indirects through `FlagStringer`, so that is the best place to customize flag rendering.

`FlagStringer` was not called for slice flags because their help output differs from other flags in two ways: there can be multiple default values, and the flag name is shown two times to indicate that the flag can be specified multiple times.

To make multiple values work in the `FlagStringer`, I simply changed `GetValue()` to return all values. This should be OK since the purpose of GetValue() is rendering documentation.

Showing the flag more than once is achieved through a new interface, `DocGenerationSliceFlag`, which the `FlagStringer` uses to decide whether the flag is a slice flag type. I'm not entirely happy with adding a new interface, but
it seems like the only fully backwards-compatible solution.


Patch:
diff --git a/flag.go b/flag.go
index 7b5ec49..a6fea1c 100644
--- a/flag.go
+++ b/flag.go
@@ -129,6 +129,14 @@ type DocGenerationFlag interface {
 	GetEnvVars() []string
 }
 
+// DocGenerationSliceFlag extends DocGenerationFlag for slice-based flags.
+type DocGenerationSliceFlag interface {
+	DocGenerationFlag
+
+	// IsSliceFlag returns true for flags that can be given multiple times.
+	IsSliceFlag() bool
+}
+
 // VisibleFlag is an interface that allows to check if a flag is visible
 type VisibleFlag interface {
 	Flag
@@ -325,24 +333,13 @@ func stringifyFlag(f Flag) string {
 
 	usageWithDefault := strings.TrimSpace(usage + defaultValueString)
 
-	return withEnvHint(df.GetEnvVars(),
-		fmt.Sprintf("%s\t%s", prefixedNames(df.Names(), placeholder), usageWithDefault))
-}
-
-func stringifySliceFlag(usage string, names, defaultVals []string) string {
-	placeholder, usage := unquoteUsage(usage)
-	if placeholder == "" {
-		placeholder = defaultPlaceholder
-	}
-
-	defaultVal := ""
-	if len(defaultVals) > 0 {
-		defaultVal = fmt.Sprintf(formatDefault("%s"), strings.Join(defaultVals, ", "))
+	pn := prefixedNames(df.Names(), placeholder)
+	sliceFlag, ok := f.(DocGenerationSliceFlag)
+	if ok && sliceFlag.IsSliceFlag() {
+		pn = pn + " [ " + pn + " ]"
 	}
 
-	usageWithDefault := strings.TrimSpace(fmt.Sprintf("%s%s", usage, defaultVal))
-	pn := prefixedNames(names, placeholder)
-	return fmt.Sprintf("%s [ %s ]\t%s", pn, pn, usageWithDefault)
+	return withEnvHint(df.GetEnvVars(), fmt.Sprintf("%s\t%s", pn, usageWithDefault))
 }
 
 func hasFlag(flags []Flag, fl Flag) bool {
diff --git a/flag_float64_slice.go b/flag_float64_slice.go
index 2cb5e4a..413aa50 100644
--- a/flag_float64_slice.go
+++ b/flag_float64_slice.go
@@ -83,7 +83,7 @@ func (f *Float64Slice) Get() interface{} {
 // String returns a readable representation of this value
 // (for usage defaults)
 func (f *Float64SliceFlag) String() string {
-	return withEnvHint(f.GetEnvVars(), f.stringify())
+	return FlagStringer(f)
 }
 
 // TakesValue returns true if the flag takes a value, otherwise false
@@ -104,10 +104,13 @@ func (f *Float64SliceFlag) GetCategory() string {
 // GetValue returns the flags value as string representation and an empty
 // string if the flag takes no value at all.
 func (f *Float64SliceFlag) GetValue() string {
-	if f.Value != nil {
-		return f.Value.String()
+	var defaultVals []string
+	if f.Value != nil && len(f.Value.Value()) > 0 {
+		for _, i := range f.Value.Value() {
+			defaultVals = append(defaultVals, strings.TrimRight(strings.TrimRight(fmt.Sprintf("%f", i), "0"), "."))
+		}
 	}
-	return ""
+	return strings.Join(defaultVals, ", ")
 }
 
 // GetDefaultText returns the default text for this flag
@@ -123,6 +126,11 @@ func (f *Float64SliceFlag) GetEnvVars() []string {
 	return f.EnvVars
 }
 
+// IsSliceFlag implements DocGenerationSliceFlag.
+func (f *Float64SliceFlag) IsSliceFlag() bool {
+	return true
+}
+
 // Apply populates the flag given the flag set and environment
 func (f *Float64SliceFlag) Apply(set *flag.FlagSet) error {
 	// apply any default
@@ -169,18 +177,6 @@ func (f *Float64SliceFlag) Get(ctx *Context) []float64 {
 	return ctx.Float64Slice(f.Name)
 }
 
-func (f *Float64SliceFlag) stringify() string {
-	var defaultVals []string
-
-	if f.Value != nil && len(f.Value.Value()) > 0 {
-		for _, i := range f.Value.Value() {
-			defaultVals = append(defaultVals, strings.TrimRight(strings.TrimRight(fmt.Sprintf("%f", i), "0"), "."))
-		}
-	}
-
-	return stringifySliceFlag(f.Usage, f.Names(), defaultVals)
-}
-
 // RunAction executes flag action if set
 func (f *Float64SliceFlag) RunAction(c *Context) error {
 	if f.Action != nil {
diff --git a/flag_int64_slice.go b/flag_int64_slice.go
index d4a11b6..c45c43d 100644
--- a/flag_int64_slice.go
+++ b/flag_int64_slice.go
@@ -84,7 +84,7 @@ func (i *Int64Slice) Get() interface{} {
 // String returns a readable representation of this value
 // (for usage defaults)
 func (f *Int64SliceFlag) String() string {
-	return withEnvHint(f.GetEnvVars(), f.stringify())
+	return FlagStringer(f)
 }
 
 // TakesValue returns true of the flag takes a value, otherwise false
@@ -105,10 +105,13 @@ func (f *Int64SliceFlag) GetCategory() string {
 // GetValue returns the flags value as string representation and an empty
 // string if the flag takes no value at all.
 func (f *Int64SliceFlag) GetValue() string {
-	if f.Value != nil {
-		return f.Value.String()
+	var defaultVals []string
+	if f.Value != nil && len(f.Value.Value()) > 0 {
+		for _, i := range f.Value.Value() {
+			defaultVals = append(defaultVals, strconv.FormatInt(i, 10))
+		}
 	}
-	return ""
+	return strings.Join(defaultVals, ", ")
 }
 
 // GetDefaultText returns the default text for this flag
@@ -124,6 +127,11 @@ func (f *Int64SliceFlag) GetEnvVars() []string {
 	return f.EnvVars
 }
 
+// IsSliceFlag implements DocGenerationSliceFlag.
+func (f *Int64SliceFlag) IsSliceFlag() bool {
+	return true
+}
+
 // Apply populates the flag given the flag set and environment
 func (f *Int64SliceFlag) Apply(set *flag.FlagSet) error {
 	// apply any default
@@ -168,17 +176,6 @@ func (f *Int64SliceFlag) Get(ctx *Context) []int64 {
 	return ctx.Int64Slice(f.Name)
 }
 
-func (f *Int64SliceFlag) stringify() string {
-	var defaultVals []string
-	if f.Value != nil && len(f.Value.Value()) > 0 {
-		for _, i := range f.Value.Value() {
-			defaultVals = append(defaultVals, strconv.FormatInt(i, 10))
-		}
-	}
-
-	return stringifySliceFlag(f.Usage, f.Names(), defaultVals)
-}
-
 // RunAction executes flag action if set
 func (f *Int64SliceFlag) RunAction(c *Context) error {
 	if f.Action != nil {
diff --git a/flag_int_slice.go b/flag_int_slice.go
index 2cabe72..d4006e5 100644
--- a/flag_int_slice.go
+++ b/flag_int_slice.go
@@ -95,7 +95,7 @@ func (i *IntSlice) Get() interface{} {
 // String returns a readable representation of this value
 // (for usage defaults)
 func (f *IntSliceFlag) String() string {
-	return withEnvHint(f.GetEnvVars(), f.stringify())
+	return FlagStringer(f)
 }
 
 // TakesValue returns true of the flag takes a value, otherwise false
@@ -116,10 +116,13 @@ func (f *IntSliceFlag) GetCategory() string {
 // GetValue returns the flags value as string representation and an empty
 // string if the flag takes no value at all.
 func (f *IntSliceFlag) GetValue() string {
-	if f.Value != nil {
-		return f.Value.String()
+	var defaultVals []string
+	if f.Value != nil && len(f.Value.Value()) > 0 {
+		for _, i := range f.Value.Value() {
+			defaultVals = append(defaultVals, strconv.Itoa(i))
+		}
 	}
-	return ""
+	return strings.Join(defaultVals, ", ")
 }
 
 // GetDefaultText returns the default text for this flag
@@ -135,6 +138,11 @@ func (f *IntSliceFlag) GetEnvVars() []string {
 	return f.EnvVars
 }
 
+// IsSliceFlag implements DocGenerationSliceFlag.
+func (f *IntSliceFlag) IsSliceFlag() bool {
+	return true
+}
+
 // Apply populates the flag given the flag set and environment
 func (f *IntSliceFlag) Apply(set *flag.FlagSet) error {
 	// apply any default
@@ -188,17 +196,6 @@ func (f *IntSliceFlag) RunAction(c *Context) error {
 	return nil
 }
 
-func (f *IntSliceFlag) stringify() string {
-	var defaultVals []string
-	if f.Value != nil && len(f.Value.Value()) > 0 {
-		for _, i := range f.Value.Value() {
-			defaultVals = append(defaultVals, strconv.Itoa(i))
-		}
-	}
-
-	return stringifySliceFlag(f.Usage, f.Names(), defaultVals)
-}
-
 // IntSlice looks up the value of a local IntSliceFlag, returns
 // nil if not found
 func (cCtx *Context) IntSlice(name string) []int {
diff --git a/flag_string_slice.go b/flag_string_slice.go
index 7b46a24..baca2a2 100644
--- a/flag_string_slice.go
+++ b/flag_string_slice.go
@@ -74,7 +74,7 @@ func (s *StringSlice) Get() interface{} {
 // String returns a readable representation of this value
 // (for usage defaults)
 func (f *StringSliceFlag) String() string {
-	return withEnvHint(f.GetEnvVars(), f.stringify())
+	return FlagStringer(f)
 }
 
 // TakesValue returns true of the flag takes a value, otherwise false
@@ -95,10 +95,15 @@ func (f *StringSliceFlag) GetCategory() string {
 // GetValue returns the flags value as string representation and an empty
 // string if the flag takes no value at all.
 func (f *StringSliceFlag) GetValue() string {
-	if f.Value != nil {
-		return f.Value.String()
+	var defaultVals []string
+	if f.Value != nil && len(f.Value.Value()) > 0 {
+		for _, s := range f.Value.Value() {
+			if len(s) > 0 {
+				defaultVals = append(defaultVals, strconv.Quote(s))
+			}
+		}
 	}
-	return ""
+	return strings.Join(defaultVals, ", ")
 }
 
 // GetDefaultText returns the default text for this flag
@@ -114,6 +119,11 @@ func (f *StringSliceFlag) GetEnvVars() []string {
 	return f.EnvVars
 }
 
+// IsSliceFlag implements DocGenerationSliceFlag.
+func (f *StringSliceFlag) IsSliceFlag() bool {
+	return true
+}
+
 // Apply populates the flag given the flag set and environment
 func (f *StringSliceFlag) Apply(set *flag.FlagSet) error {
 	// apply any default
@@ -158,19 +168,6 @@ func (f *StringSliceFlag) Get(ctx *Context) []string {
 	return ctx.StringSlice(f.Name)
 }
 
-func (f *StringSliceFlag) stringify() string {
-	var defaultVals []string
-	if f.Value != nil && len(f.Value.Value()) > 0 {
-		for _, s := range f.Value.Value() {
-			if len(s) > 0 {
-				defaultVals = append(defaultVals, strconv.Quote(s))
-			}
-		}
-	}
-
-	return stringifySliceFlag(f.Usage, f.Names(), defaultVals)
-}
-
 // RunAction executes flag action if set
 func (f *StringSliceFlag) RunAction(c *Context) error {
 	if f.Action != nil {
diff --git a/flag_uint64_slice.go b/flag_uint64_slice.go
index e60c3ea..61bb30b 100644
--- a/flag_uint64_slice.go
+++ b/flag_uint64_slice.go
@@ -88,7 +88,7 @@ func (i *Uint64Slice) Get() interface{} {
 // String returns a readable representation of this value
 // (for usage defaults)
 func (f *Uint64SliceFlag) String() string {
-	return withEnvHint(f.GetEnvVars(), f.stringify())
+	return FlagStringer(f)
 }
 
 // TakesValue returns true of the flag takes a value, otherwise false
@@ -109,10 +109,13 @@ func (f *Uint64SliceFlag) GetCategory() string {
 // GetValue returns the flags value as string representation and an empty
 // string if the flag takes no value at all.
 func (f *Uint64SliceFlag) GetValue() string {
-	if f.Value != nil {
-		return f.Value.String()
+	var defaultVals []string
+	if f.Value != nil && len(f.Value.Value()) > 0 {
+		for _, i := range f.Value.Value() {
+			defaultVals = append(defaultVals, strconv.FormatUint(i, 10))
+		}
 	}
-	return ""
+	return strings.Join(defaultVals, ", ")
 }
 
 // GetDefaultText returns the default text for this flag
@@ -128,6 +131,11 @@ func (f *Uint64SliceFlag) GetEnvVars() []string {
 	return f.EnvVars
 }
 
+// IsSliceFlag implements DocGenerationSliceFlag.
+func (f *Uint64SliceFlag) IsSliceFlag() bool {
+	return true
+}
+
 // Apply populates the flag given the flag set and environment
 func (f *Uint64SliceFlag) Apply(set *flag.FlagSet) error {
 	// apply any default
@@ -172,17 +180,6 @@ func (f *Uint64SliceFlag) Get(ctx *Context) []uint64 {
 	return ctx.Uint64Slice(f.Name)
 }
 
-func (f *Uint64SliceFlag) stringify() string {
-	var defaultVals []string
-	if f.Value != nil && len(f.Value.Value()) > 0 {
-		for _, i := range f.Value.Value() {
-			defaultVals = append(defaultVals, strconv.FormatUint(i, 10))
-		}
-	}
-
-	return stringifySliceFlag(f.Usage, f.Names(), defaultVals)
-}
-
 // Uint64Slice looks up the value of a local Uint64SliceFlag, returns
 // nil if not found
 func (cCtx *Context) Uint64Slice(name string) []uint64 {
diff --git a/flag_uint_slice.go b/flag_uint_slice.go
index 350b29c..363aa65 100644
--- a/flag_uint_slice.go
+++ b/flag_uint_slice.go
@@ -99,7 +99,7 @@ func (i *UintSlice) Get() interface{} {
 // String returns a readable representation of this value
 // (for usage defaults)
 func (f *UintSliceFlag) String() string {
-	return withEnvHint(f.GetEnvVars(), f.stringify())
+	return FlagStringer(f)
 }
 
 // TakesValue returns true of the flag takes a value, otherwise false
@@ -120,10 +120,13 @@ func (f *UintSliceFlag) GetCategory() string {
 // GetValue returns the flags value as string representation and an empty
 // string if the flag takes no value at all.
 func (f *UintSliceFlag) GetValue() string {
-	if f.Value != nil {
-		return f.Value.String()
+	var defaultVals []string
+	if f.Value != nil && len(f.Value.Value()) > 0 {
+		for _, i := range f.Value.Value() {
+			defaultVals = append(defaultVals, strconv.FormatUint(uint64(i), 10))
+		}
 	}
-	return ""
+	return strings.Join(defaultVals, ", ")
 }
 
 // GetDefaultText returns the default text for this flag
@@ -139,6 +142,11 @@ func (f *UintSliceFlag) GetEnvVars() []string {
 	return f.EnvVars
 }
 
+// IsSliceFlag implements DocGenerationSliceFlag.
+func (f *UintSliceFlag) IsSliceFlag() bool {
+	return true
+}
+
 // Apply populates the flag given the flag set and environment
 func (f *UintSliceFlag) Apply(set *flag.FlagSet) error {
 	// apply any default
@@ -183,17 +191,6 @@ func (f *UintSliceFlag) Get(ctx *Context) []uint {
 	return ctx.UintSlice(f.Name)
 }
 
-func (f *UintSliceFlag) stringify() string {
-	var defaultVals []string
-	if f.Value != nil && len(f.Value.Value()) > 0 {
-		for _, i := range f.Value.Value() {
-			defaultVals = append(defaultVals, strconv.FormatUint(uint64(i), 10))
-		}
-	}
-
-	return stringifySliceFlag(f.Usage, f.Names(), defaultVals)
-}
-
 // UintSlice looks up the value of a local UintSliceFlag, returns
 // nil if not found
 func (cCtx *Context) UintSlice(name string) []uint {


Test patch:
diff --git a/flag_test.go b/flag_test.go
index ddd3e67..06f948c 100644
--- a/flag_test.go
+++ b/flag_test.go
@@ -307,12 +307,12 @@ func TestFlagStringifying(t *testing.T) {
 		{
 			name:     "float64-slice-flag",
 			fl:       &Float64SliceFlag{Name: "pizzas"},
-			expected: "--pizzas value\t",
+			expected: "--pizzas value [ --pizzas value ]\t",
 		},
 		{
 			name:     "float64-slice-flag-with-default-text",
 			fl:       &Float64SliceFlag{Name: "pepperonis", DefaultText: "shaved"},
-			expected: "--pepperonis value\t(default: shaved)",
+			expected: "--pepperonis value [ --pepperonis value ]\t(default: shaved)",
 		},
 		{
 			name:     "generic-flag",
@@ -337,7 +337,7 @@ func TestFlagStringifying(t *testing.T) {
 		{
 			name:     "int-slice-flag",
 			fl:       &IntSliceFlag{Name: "pencils"},
-			expected: "--pencils value\t",
+			expected: "--pencils value [ --pencils value ]\t",
 		},
 		{
 			name:     "int-slice-flag-with-default-text",
@@ -347,7 +347,7 @@ func TestFlagStringifying(t *testing.T) {
 		{
 			name:     "uint-slice-flag",
 			fl:       &UintSliceFlag{Name: "pencils"},
-			expected: "--pencils value\t",
+			expected: "--pencils value [ --pencils value ]\t",
 		},
 		{
 			name:     "uint-slice-flag-with-default-text",
@@ -367,22 +367,22 @@ func TestFlagStringifying(t *testing.T) {
 		{
 			name:     "int64-slice-flag",
 			fl:       &Int64SliceFlag{Name: "drawers"},
-			expected: "--drawers value\t",
+			expected: "--drawers value [ --drawers value ]\t",
 		},
 		{
 			name:     "int64-slice-flag-with-default-text",
 			fl:       &Int64SliceFlag{Name: "handles", DefaultText: "-2"},
-			expected: "--handles value\t(default: -2)",
+			expected: "--handles value [ --handles value ]\t(default: -2)",
 		},
 		{
 			name:     "uint64-slice-flag",
 			fl:       &Uint64SliceFlag{Name: "drawers"},
-			expected: "--drawers value\t",
+			expected: "--drawers value [ --drawers value ]\t",
 		},
 		{
 			name:     "uint64-slice-flag-with-default-text",
 			fl:       &Uint64SliceFlag{Name: "handles", DefaultText: "-2"},
-			expected: "--handles value\t(default: -2)",
+			expected: "--handles value [ --handles value ]\t(default: -2)",
 		},
 		{
 			name:     "path-flag",
@@ -407,12 +407,12 @@ func TestFlagStringifying(t *testing.T) {
 		{
 			name:     "string-slice-flag",
 			fl:       &StringSliceFlag{Name: "meow-sounds"},
-			expected: "--meow-sounds value\t",
+			expected: "--meow-sounds value [ --meow-sounds value ]\t",
 		},
 		{
 			name:     "string-slice-flag-with-default-text",
 			fl:       &StringSliceFlag{Name: "moo-sounds", DefaultText: "awoo"},
-			expected: "--moo-sounds value\t(default: awoo)",
+			expected: "--moo-sounds value [ --moo-sounds value ]\t(default: awoo)",
 		},
 		{
 			name:     "timestamp-flag",