v2 bug: fishAddFileFlag doesn't work with PathFlag

[ErinCall]

my urfave/cli version is:

2.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

By default, fishAddFileFlag adds -f to the complete command, indicating that the input to the flag shouldn't be a filename. Naturally, a PathFlag should accept a filename, but fishAddFileFlag only checks GenericFlag, StringFlag, and StringSliceFlag.

To reproduce

Create a main.go:

package main

import "fmt"
import "github.com/urfave/cli/v2"

func main() {
	app := &cli.App{
		Flags: []cli.Flag{
			&cli.PathFlag{
				Name:      "path-file",
				TakesFile: true,
			},
			&cli.StringFlag{
				Name:      "string-file",
				TakesFile: true,
			},
		},
	}

	output, _ := app.ToFishCompletion()
	fmt.Println(output)
}

Observed behavior

run go run ./main.go. The output will include:

complete -c  -n '__fish__no_subcommand' -f -l path-file -r
complete -c  -n '__fish__no_subcommand' -l string-file -r

Expected behavior

The first line, for path-file, shouldn't have -f.

Want to fix this yourself?

Sure! I could add another case statement for PathFlag, but using reflect might be more future-proof. Do you have a preference?

Run go env and go version

Output of those commands

go version

go version go1.13.6 darwin/amd64

go env

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/erincalling/Library/Caches/go-build"
GOENV="/Users/erincalling/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/erincalling/go/"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.6/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.6/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/erincalling/code/futzing/cli-pathflag/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/bj/13jz98m53fl992t2sn3h0q2r0000gn/T/go-build047644300=/tmp/go-build -gno-record-gcc-switches -fno-common"

[coilysiren]

Sure! I could add another case statement for PathFlag, but using reflect might be more future-proof. Do you have a preference?

I think a case statement would be preferred over a reflection here, although I feel like you should be able to check f.TakesFile without needing a case statement 🤔 I'm imagining something like this:

type takesFile struct {
  TakesFile bool
}

# ...

f, ok := flag.(takesFile); ok {
  if f.TakesFile {
    return
  }
}

let me know if that makes sense! Either way, I'm interested in whatever you come up with!

[stale]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[ErinCall]

Sorry for letting this get stale! I’ve opened a PR.

[stale]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.