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

Support PathFlag.TakesFile in fish completion #1198

merged 5 commits into from Oct 31, 2020


Copy link

@ErinCall ErinCall commented Oct 13, 2020

What type of PR is this?

  • bug
  • cleanup
  • documentation
  • feature

What this PR does / why we need it:

Flags of type PathFlag weren't processed correctly when generating fish completions—the generated complete command told fish that the flag doesn't take a filename, even when TakesFile is true.

  • fish.go now respects the TakesFile field, regardless of the flag's underlying type.
    • The first commit on this branch (5bb54ac) is a simpler solution to the original bug: it simply appends PathFlag to the list of explicit types in the switch statement.
    • The second commit refactors the switch statement, using reflection rather than explicit list of types. It's more flexible, but possibly harder to follow. I added some comments to offset that caveat, but I won't complain if you prefer the simpler design.
  • fish_test.go has a test case for a PathFlag with TakesFile = true.
  • has a corresponding change in its fixture data.

Which issue(s) this PR fixes:

Fixes #1156

Special notes for your reviewer:

@lynncyrin had suggested a lighter-weight form of reflection, but I wasn't able to make the type assertion work. AFAIK go's type system only deals in interfaces and specific, concrete types.

Release Notes

fish completion now respects TakesFile on PathFlags
ErinCall added 3 commits Jun 23, 2020
Performing reflection on the given flag ensures that fishAddFileFlag
will behave correctly if any flag types get a TakesFile field in the
@ErinCall ErinCall requested a review from urfave/cli as a code owner Oct 13, 2020
@ErinCall ErinCall requested review from saschagrunert and rliebz and removed request for urfave/cli Oct 13, 2020
Copy link

@saschagrunert saschagrunert left a comment

Nice, LGTM

Copy link

@rliebz rliebz left a comment

Thanks for putting this together!

Left a couple comments with some options for how to go forward. Feel free to head down either path.

fish.go Outdated Show resolved Hide resolved
fish.go Outdated Show resolved Hide resolved
ErinCall added 2 commits Oct 22, 2020
This reverts commit fcce511.
rliebz approved these changes Oct 22, 2020
Copy link

@asahasrabuddhe asahasrabuddhe left a comment


@rliebz rliebz merged commit 3be15f7 into urfave:master Oct 31, 2020
12 checks passed
12 checks passed
ubuntu-latest @ Go 1.12 ubuntu-latest @ Go 1.12
ubuntu-latest @ Go 1.13 ubuntu-latest @ Go 1.13
ubuntu-latest @ Go 1.14 ubuntu-latest @ Go 1.14
macos-latest @ Go 1.12 macos-latest @ Go 1.12
macos-latest @ Go 1.13 macos-latest @ Go 1.13
macos-latest @ Go 1.14 macos-latest @ Go 1.14
windows-latest @ Go 1.12 windows-latest @ Go 1.12
windows-latest @ Go 1.13 windows-latest @ Go 1.13
windows-latest @ Go 1.14 windows-latest @ Go 1.14
test-docs test-docs
codecov/patch 100.00% of diff hit (target 71.61%)
codecov/project 71.75% (+0.14%) compared to e062e73
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants