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
merged 5 commits into from Oct 31, 2020

Conversation

ErinCall
Copy link
Contributor

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.
  • expected-fish-full.fish 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

Performing reflection on the given flag ensures that fishAddFileFlag
will behave correctly if any flag types get a TakesFile field in the
future.
@ErinCall ErinCall requested a review from a team as a code owner October 13, 2020 20:25
@ErinCall ErinCall requested review from saschagrunert and rliebz and removed request for a team October 13, 2020 20:25
saschagrunert
saschagrunert previously approved these changes Oct 14, 2020
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM

Copy link
Member

@rliebz rliebz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@asahasrabuddhe asahasrabuddhe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rliebz rliebz merged commit 3be15f7 into urfave:master Oct 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v2 bug: fishAddFileFlag doesn't work with PathFlag
4 participants