Add SliceFlag wrapper and fix bugs in existing implementations

The SliceFlag implementation and associated aliases (MultiStringFlag, etc) extend the existing slice implementations (StringSliceFlag, etc) to support actual slices as the flag value and destination. This change also fixes various bugs in the existing implementation. Notably, the StringSliceFlag.Apply implementation would modify the input (default) Value, if an env var was set, and no destination was provided. The bugs fixed in the other three implementations were all already fixed in either StringSliceFlag, or in one case (ignoring empty env var) in Float64SliceFlag.
urfave · Jun 6, 2022 · e77dd7b · e77dd7b
1 parent 947f989
commit e77dd7b
Show file tree

Hide file tree

Showing 7 changed files with 1,473 additions and 56 deletions.
diff --git a/flag_float64_slice.go b/flag_float64_slice.go
@@ -56,7 +56,12 @@ func (f *Float64Slice) Set(value string) error {
 
 // String returns a readable representation of this value (for usage defaults)
 func (f *Float64Slice) String() string {
-	return fmt.Sprintf("%#v", f.slice)
+	v := f.slice
+	if v == nil {
+		// treat nil the same as zero length non-nil
+		v = make([]float64, 0)
+	}
+	return fmt.Sprintf("%#v", v)
 }
 
 // Serialize allows Float64Slice to fulfill Serializer
@@ -120,34 +125,60 @@ func (f *Float64SliceFlag) GetEnvVars() []string {
 
 // Apply populates the flag given the flag set and environment
 func (f *Float64SliceFlag) Apply(set *flag.FlagSet) error {
+	// apply any default
+	if f.Destination != nil && f.Value != nil {
+		f.Destination.slice = make([]float64, len(f.Value.slice))
+		copy(f.Destination.slice, f.Value.slice)
+	}
+
+	// resolve setValue (what we will assign to the set)
+	var setValue *Float64Slice
+	switch {
+	case f.Destination != nil:
+		setValue = f.Destination
+	case f.Value != nil:
+		setValue = f.Value.clone()
+	default:
+		setValue = new(Float64Slice)
+	}
+
 	if val, source, found := flagFromEnvOrFile(f.EnvVars, f.FilePath); found {
 		if val != "" {
-			f.Value = &Float64Slice{}
-
 			for _, s := range flagSplitMultiValues(val) {
-				if err := f.Value.Set(strings.TrimSpace(s)); err != nil {
-					return fmt.Errorf("could not parse %q as float64 slice value from %s for flag %s: %s", f.Value, source, f.Name, err)
+				if err := setValue.Set(strings.TrimSpace(s)); err != nil {
+					return fmt.Errorf("could not parse %q as float64 slice value from %s for flag %s: %s", val, source, f.Name, err)
 				}
 			}
 
 			// Set this to false so that we reset the slice if we then set values from
 			// flags that have already been set by the environment.
-			f.Value.hasBeenSet = false
+			setValue.hasBeenSet = false
 			f.HasBeenSet = true
 		}
 	}
 
-	if f.Value == nil {
-		f.Value = &Float64Slice{}
-	}
-	copyValue := f.Value.clone()
 	for _, name := range f.Names() {
-		set.Var(copyValue, name, f.Usage)
+		set.Var(setValue, name, f.Usage)
 	}
 
 	return nil
 }
 
+func (f *Float64SliceFlag) SetValue(slice []float64) {
+	f.Value = newSliceFlagValue(NewFloat64Slice, slice)
+}
+
+func (f *Float64SliceFlag) SetDestination(slice []float64) {
+	f.Destination = newSliceFlagValue(NewFloat64Slice, slice)
+}
+
+func (f *Float64SliceFlag) GetDestination() []float64 {
+	if destination := f.Destination; destination != nil {
+		return destination.Value()
+	}
+	return nil
+}
+
 // Get returns the flag’s value in the given Context.
 func (f *Float64SliceFlag) Get(ctx *Context) []float64 {
 	return ctx.Float64Slice(f.Name)
@@ -165,7 +196,7 @@ func (cCtx *Context) Float64Slice(name string) []float64 {
 func lookupFloat64Slice(name string, set *flag.FlagSet) []float64 {
 	f := set.Lookup(name)
 	if f != nil {
-		if slice, ok := f.Value.(*Float64Slice); ok {
+		if slice, ok := unwrapFlagValue(f.Value).(*Float64Slice); ok {
 			return slice.Value()
 		}
 	}

diff --git a/flag_int64_slice.go b/flag_int64_slice.go
@@ -57,7 +57,12 @@ func (i *Int64Slice) Set(value string) error {
 
 // String returns a readable representation of this value (for usage defaults)
 func (i *Int64Slice) String() string {
-	return fmt.Sprintf("%#v", i.slice)
+	v := i.slice
+	if v == nil {
+		// treat nil the same as zero length non-nil
+		v = make([]int64, 0)
+	}
+	return fmt.Sprintf("%#v", v)
 }
 
 // Serialize allows Int64Slice to fulfill Serializer
@@ -121,32 +126,58 @@ func (f *Int64SliceFlag) GetEnvVars() []string {
 
 // Apply populates the flag given the flag set and environment
 func (f *Int64SliceFlag) Apply(set *flag.FlagSet) error {
-	if val, source, found := flagFromEnvOrFile(f.EnvVars, f.FilePath); found {
-		f.Value = &Int64Slice{}
+	// apply any default
+	if f.Destination != nil && f.Value != nil {
+		f.Destination.slice = make([]int64, len(f.Value.slice))
+		copy(f.Destination.slice, f.Value.slice)
+	}
+
+	// resolve setValue (what we will assign to the set)
+	var setValue *Int64Slice
+	switch {
+	case f.Destination != nil:
+		setValue = f.Destination
+	case f.Value != nil:
+		setValue = f.Value.clone()
+	default:
+		setValue = new(Int64Slice)
+	}
 
+	if val, source, ok := flagFromEnvOrFile(f.EnvVars, f.FilePath); ok && val != "" {
 		for _, s := range flagSplitMultiValues(val) {
-			if err := f.Value.Set(strings.TrimSpace(s)); err != nil {
+			if err := setValue.Set(strings.TrimSpace(s)); err != nil {
 				return fmt.Errorf("could not parse %q as int64 slice value from %s for flag %s: %s", val, source, f.Name, err)
 			}
 		}
 
 		// Set this to false so that we reset the slice if we then set values from
 		// flags that have already been set by the environment.
-		f.Value.hasBeenSet = false
+		setValue.hasBeenSet = false
 		f.HasBeenSet = true
 	}
 
-	if f.Value == nil {
-		f.Value = &Int64Slice{}
-	}
-	copyValue := f.Value.clone()
 	for _, name := range f.Names() {
-		set.Var(copyValue, name, f.Usage)
+		set.Var(setValue, name, f.Usage)
 	}
 
 	return nil
 }
 
+func (f *Int64SliceFlag) SetValue(slice []int64) {
+	f.Value = newSliceFlagValue(NewInt64Slice, slice)
+}
+
+func (f *Int64SliceFlag) SetDestination(slice []int64) {
+	f.Destination = newSliceFlagValue(NewInt64Slice, slice)
+}
+
+func (f *Int64SliceFlag) GetDestination() []int64 {
+	if destination := f.Destination; destination != nil {
+		return destination.Value()
+	}
+	return nil
+}
+
 // Get returns the flag’s value in the given Context.
 func (f *Int64SliceFlag) Get(ctx *Context) []int64 {
 	return ctx.Int64Slice(f.Name)
@@ -164,7 +195,7 @@ func (cCtx *Context) Int64Slice(name string) []int64 {
 func lookupInt64Slice(name string, set *flag.FlagSet) []int64 {
 	f := set.Lookup(name)
 	if f != nil {
-		if slice, ok := f.Value.(*Int64Slice); ok {
+		if slice, ok := unwrapFlagValue(f.Value).(*Int64Slice); ok {
 			return slice.Value()
 		}
 	}

diff --git a/flag_int_slice.go b/flag_int_slice.go
@@ -68,7 +68,12 @@ func (i *IntSlice) Set(value string) error {
 
 // String returns a readable representation of this value (for usage defaults)
 func (i *IntSlice) String() string {
-	return fmt.Sprintf("%#v", i.slice)
+	v := i.slice
+	if v == nil {
+		// treat nil the same as zero length non-nil
+		v = make([]int, 0)
+	}
+	return fmt.Sprintf("%#v", v)
 }
 
 // Serialize allows IntSlice to fulfill Serializer
@@ -132,32 +137,58 @@ func (f *IntSliceFlag) GetEnvVars() []string {
 
 // Apply populates the flag given the flag set and environment
 func (f *IntSliceFlag) Apply(set *flag.FlagSet) error {
-	if val, source, found := flagFromEnvOrFile(f.EnvVars, f.FilePath); found {
-		f.Value = &IntSlice{}
+	// apply any default
+	if f.Destination != nil && f.Value != nil {
+		f.Destination.slice = make([]int, len(f.Value.slice))
+		copy(f.Destination.slice, f.Value.slice)
+	}
+
+	// resolve setValue (what we will assign to the set)
+	var setValue *IntSlice
+	switch {
+	case f.Destination != nil:
+		setValue = f.Destination
+	case f.Value != nil:
+		setValue = f.Value.clone()
+	default:
+		setValue = new(IntSlice)
+	}
 
+	if val, source, ok := flagFromEnvOrFile(f.EnvVars, f.FilePath); ok && val != "" {
 		for _, s := range flagSplitMultiValues(val) {
-			if err := f.Value.Set(strings.TrimSpace(s)); err != nil {
+			if err := setValue.Set(strings.TrimSpace(s)); err != nil {
 				return fmt.Errorf("could not parse %q as int slice value from %s for flag %s: %s", val, source, f.Name, err)
 			}
 		}
 
 		// Set this to false so that we reset the slice if we then set values from
 		// flags that have already been set by the environment.
-		f.Value.hasBeenSet = false
+		setValue.hasBeenSet = false
 		f.HasBeenSet = true
 	}
 
-	if f.Value == nil {
-		f.Value = &IntSlice{}
-	}
-	copyValue := f.Value.clone()
 	for _, name := range f.Names() {
-		set.Var(copyValue, name, f.Usage)
+		set.Var(setValue, name, f.Usage)
 	}
 
 	return nil
 }
 
+func (f *IntSliceFlag) SetValue(slice []int) {
+	f.Value = newSliceFlagValue(NewIntSlice, slice)
+}
+
+func (f *IntSliceFlag) SetDestination(slice []int) {
+	f.Destination = newSliceFlagValue(NewIntSlice, slice)
+}
+
+func (f *IntSliceFlag) GetDestination() []int {
+	if destination := f.Destination; destination != nil {
+		return destination.Value()
+	}
+	return nil
+}
+
 // Get returns the flag’s value in the given Context.
 func (f *IntSliceFlag) Get(ctx *Context) []int {
 	return ctx.IntSlice(f.Name)
@@ -175,7 +206,7 @@ func (cCtx *Context) IntSlice(name string) []int {
 func lookupIntSlice(name string, set *flag.FlagSet) []int {
 	f := set.Lookup(name)
 	if f != nil {
-		if slice, ok := f.Value.(*IntSlice); ok {
+		if slice, ok := unwrapFlagValue(f.Value).(*IntSlice); ok {
 			return slice.Value()
 		}
 	}

diff --git a/flag_string_slice.go b/flag_string_slice.go
@@ -115,48 +115,58 @@ func (f *StringSliceFlag) GetEnvVars() []string {
 
 // Apply populates the flag given the flag set and environment
 func (f *StringSliceFlag) Apply(set *flag.FlagSet) error {
-
+	// apply any default
 	if f.Destination != nil && f.Value != nil {
 		f.Destination.slice = make([]string, len(f.Value.slice))
 		copy(f.Destination.slice, f.Value.slice)
+	}
 
+	// resolve setValue (what we will assign to the set)
+	var setValue *StringSlice
+	switch {
+	case f.Destination != nil:
+		setValue = f.Destination
+	case f.Value != nil:
+		setValue = f.Value.clone()
+	default:
+		setValue = new(StringSlice)
 	}
 
 	if val, source, found := flagFromEnvOrFile(f.EnvVars, f.FilePath); found {
-		if f.Value == nil {
-			f.Value = &StringSlice{}
-		}
-		destination := f.Value
-		if f.Destination != nil {
-			destination = f.Destination
-		}
-
 		for _, s := range flagSplitMultiValues(val) {
-			if err := destination.Set(strings.TrimSpace(s)); err != nil {
+			if err := setValue.Set(strings.TrimSpace(s)); err != nil {
 				return fmt.Errorf("could not parse %q as string value from %s for flag %s: %s", val, source, f.Name, err)
 			}
 		}
 
 		// Set this to false so that we reset the slice if we then set values from
 		// flags that have already been set by the environment.
-		destination.hasBeenSet = false
+		setValue.hasBeenSet = false
 		f.HasBeenSet = true
 	}
 
-	if f.Value == nil {
-		f.Value = &StringSlice{}
-	}
-	setValue := f.Destination
-	if f.Destination == nil {
-		setValue = f.Value.clone()
-	}
 	for _, name := range f.Names() {
 		set.Var(setValue, name, f.Usage)
 	}
 
 	return nil
 }
 
+func (f *StringSliceFlag) SetValue(slice []string) {
+	f.Value = newSliceFlagValue(NewStringSlice, slice)
+}
+
+func (f *StringSliceFlag) SetDestination(slice []string) {
+	f.Destination = newSliceFlagValue(NewStringSlice, slice)
+}
+
+func (f *StringSliceFlag) GetDestination() []string {
+	if destination := f.Destination; destination != nil {
+		return destination.Value()
+	}
+	return nil
+}
+
 // Get returns the flag’s value in the given Context.
 func (f *StringSliceFlag) Get(ctx *Context) []string {
 	return ctx.StringSlice(f.Name)
@@ -174,7 +184,7 @@ func (cCtx *Context) StringSlice(name string) []string {
 func lookupStringSlice(name string, set *flag.FlagSet) []string {
 	f := set.Lookup(name)
 	if f != nil {
-		if slice, ok := f.Value.(*StringSlice); ok {
+		if slice, ok := unwrapFlagValue(f.Value).(*StringSlice); ok {
 			return slice.Value()
 		}
 	}