v2_bug_fixing_1121 : Now StringSliceFlag, Value(default) value set in Destination

[atif-konasl]

Motivation

Value(default) does not set in Destination field in StringSliceFlag.

Findings

• No slice of variables like []string, []int, []float can be set in flag.go in flag package in golang.
• No set.StringVar(f.Destination, name, f.Value, f.Usage) this type of method for slice of string in flag.go in flag package.

Changes

When Destination field set as a StringSlice in StringSliceFlag then we need to set Value(StringSlice) in Destination as default. But flag.go in flag package does not support this.
For that reason we need to set Value in Destination before call set.Var(f.Destination, name, f.Usage)

Before Change :

// Apply populates the flag given the flag set and environment
func (f *StringSliceFlag) Apply(set *flag.FlagSet) error {
	if val, ok := flagFromEnvOrFile(f.EnvVars, f.FilePath); ok {
		f.Value = &StringSlice{}
		destination := f.Value
		if f.Destination != nil {
			destination = f.Destination
		}

		for _, s := range strings.Split(val, ",") {
			if err := destination.Set(strings.TrimSpace(s)); err != nil {
				return fmt.Errorf("could not parse %q as string value for flag %s: %s", val, 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
		f.HasBeenSet = true
	}

	for _, name := range f.Names() {
		if f.Value == nil {
			f.Value = &StringSlice{}
		}

		if f.Destination != nil {
			set.Var(f.Destination, name, f.Usage)
			continue
		}

		set.Var(f.Value, name, f.Usage)
	}
	return nil
}

After Change :

// Apply populates the flag given the flag set and environment
func (f *StringSliceFlag) Apply(set *flag.FlagSet) error {
	if val, ok := flagFromEnvOrFile(f.EnvVars, f.FilePath); ok {
		f.Value = &StringSlice{}
		destination := f.Value
		if f.Destination != nil {
			destination = f.Destination
		}

		for _, s := range strings.Split(val, ",") {
			if err := destination.Set(strings.TrimSpace(s)); err != nil {
				return fmt.Errorf("could not parse %q as string value for flag %s: %s", val, 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
		f.HasBeenSet = true
	}

	for _, name := range f.Names() {
		if f.Value == nil {
			f.Value = &StringSlice{}
		}

		if f.Destination != nil {
			f.Destination.slice = make([]string, len(f.Value.slice))
			copy(f.Destination.slice, f.Value.slice)
			set.Var(f.Destination, name, f.Usage)
			continue
		}

		set.Var(f.Value, name, f.Usage)
	}

	return nil
}

Run go env

GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/atif/.cache/go-build"
GOENV="/home/atif/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/atif/WorkStation/Lukso/CodeBase"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build805853841=/tmp/go-build -gno-record-gcc-switches"

[saschagrunert]

LGTM

[saschagrunert]

Can we add a test for that?

[atif-konasl]

Yes I will add a test.

[atif-konasl]

@saschagrunert and @rliebz , i added a test in app_test.go. Pls check.

[saschagrunert]

Hm, @urfave/cli I think the CI is broken here but I'm not sure if it's related to this change 🤔

[coilysiren]

echoing my comment on #1127, this PR looks like it solves the same issue as that one.

@sgoroshko & @atif-konasl can you both have a conversation about whose solution you think should move forward? We can't merge both

[coilysiren]

feel free to re-request me when the duplication between #1127 and #1123 is resolved!

[coilysiren]

With respect to CI failing, @atif-konasl try removing the special characters from your branch name. That will require you to create a new PR, most likely.