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

IntSliceFlag and friends don't work correctly with destination (+ behavior quirks relating to value) #1410

Closed
3 tasks done
joeycumines opened this issue Jun 7, 2022 · 0 comments · Fixed by #1409
Closed
3 tasks done
Labels
area/v2 relates to / is being considered for v2 kind/bug describes or fixes a bug

Comments

@joeycumines
Copy link
Contributor

joeycumines commented Jun 7, 2022

My urfave/cli version is

v2.8.1

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.

Describe the bug

I've noticed a some issues / inconsistencies, while implementing #1409. I'm creating this issue just to make it clear what they are.

  • It appears Destination fields were added to the IntSliceFlag, Int64SliceFlag, and Float64SliceFlag structs, without actually implementing the necessary behavior
  • The StringSliceFlag.Value (default value) field may be modified by values passed in by environment variables
  • It doesn't seem like a problem, but the Value field seems like it should never be modified (e.g. don't initialise it if it's nil / use a local variable instead)

Past issues/PRs that may be relevant: #1235, #981, #1178

Currently open issues that may be relevant: #1224, #1143

To reproduce

N/A, but take a look at #1409, particularly the existing test I modified (for the second issue)

Observed behavior

See above

Expected behavior

The value / destination support of IntSliceFlag etc should be equivalent to StringSliceFlag. That said, there's a bit of a question around handling the (set) empty string env var case, however. See the note on my PR.

Run go version and paste its output here

go version go1.18.2 linux/amd64

Run go env and paste its output here

GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/joey/.cache/go-build"
GOENV="/home/joey/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/joey/under/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/joey/under/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/f/dev/urfave-cli/go.mod"
GOWORK=""
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-build1554591512=/tmp/go-build -gno-record-gcc-switches"
@joeycumines joeycumines added area/v2 relates to / is being considered for v2 kind/bug describes or fixes a bug status/triage maintainers still need to look into this labels Jun 7, 2022
@meatballhat meatballhat removed the status/triage maintainers still need to look into this label Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 kind/bug describes or fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants