Skip to content
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.

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

v2 bug: ( StringSliceFlag, Value(default) does not set value in Destination ) #1121

Closed
sgoroshko opened this issue Apr 28, 2020 · 4 comments
Closed

Comments

@sgoroshko
Copy link
Contributor

@sgoroshko sgoroshko commented Apr 28, 2020

my urfave/cli version is

github.com/urfave/cli/v2 v2.2.0

Checklist

  • Are you running the latest v2 release? The list of releases is here.
  • Did you check the manual for your release? The v2 manual is here
  • Did you perform a search about this problem? Here's the Github guide about searching.

Dependency Management

  • My project is using go modules.
  • My project is using vendoring.
  • My project is automatically downloading the latest version.
  • I am unsure of what my dependency management setup is.

Describe the bug

Default value does not setting in destination.

To reproduce

package main

import (
	"github.com/urfave/cli/v2"
	"log"
	"os"
)

type config struct {
	Countries cli.StringSlice
}

func main() {
	cfg := new(config)

	app := cli.NewApp()
	app.Flags = []cli.Flag{
		&cli.StringSliceFlag{
			Name:        "country",
			Aliases:     []string{"c"},
			Value:       cli.NewStringSlice("CA", "US"),
			Destination: &cfg.Countries,
		},
	}
	app.Action = func(c *cli.Context) error {
		log.Printf("%+v", cfg)
		return nil
	}
	_ = app.Run(os.Args)
}

Observed behavior

go run main.go 
2020/04/28 22:54:56 &{Countries:{slice:[] hasBeenSet:false}}

Expected behavior

go run main.go 
2020/04/28 22:54:56 &{Countries:{slice:[CA US] hasBeenSet:false}}

Additional context

go run main.go -c UA
2020/04/28 22:55:00 &{Countries:{slice:[UA] hasBeenSet:true}}

Want to fix this yourself?

set default values manually
cfg := &config{Countries: []string{"CA", "US"}}

# paste `go version` output in here

go version go1.14.2 darwin/amd64

Run go env and paste its output here

GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/user/Library/Caches/go-build"
GOENV="/Users/user/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=" -mod="
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/user/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/local/Cellar/go/1.14.2_1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.2_1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/user/cli_test/go.mod"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/n5/7nnknbgd727_w4s8yv3z9hyh0000gn/T/go-build026345303=/tmp/go-build -gno-record-gcc-switches -fno-common"
@sgoroshko sgoroshko changed the title v2 bug: ( StringSliceFlag, Value(default) value in Destination ) v2 bug: ( StringSliceFlag, Value(default) does not set value in Destination ) Apr 28, 2020
@saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Apr 29, 2020

Yes, this seems like a bug I reproduced it on my side. @sgoroshko do you think that you're going to open a PR to address that issue?

@sgoroshko
Copy link
Contributor Author

@sgoroshko sgoroshko commented Apr 29, 2020

by analog with the implementation of other flags

flag.go:839 func (f *FlagSet) Var(...
default value already must be set in value flag.Value argument

and it's set, example for int, by flag.go:649 func (f *FlagSet) IntVar
in newIntValue function

so, for me solution can be like that
flag_string_slice.go:138

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

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

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

also, think it's problem actual for all slice like flags.

@sgoroshko
Copy link
Contributor Author

@sgoroshko sgoroshko commented May 4, 2020

can u review my PR?

@lynncyrin
Copy link
Member

@lynncyrin lynncyrin commented May 4, 2020

Hey @sgoroshko in case you missed in, you'll need to resolve the duplication between your PR and #1123

@lynncyrin lynncyrin closed this in 196b222 Jun 4, 2020
lynncyrin added a commit that referenced this issue Jun 4, 2020
fix #1121(StringSliceFlag set default value into destination)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants