Skip to content

fix: honor TakesFile for every flag type in fish completion - #2500

Merged
Juneezee merged 1 commit into
urfave:mainfrom
vidigoat:fix/fish-takes-file-all-flag-types
Oct 25, 2026
Merged

Juneezee merged 1 commit into
urfave:mainfrom
vidigoat:fix/fish-takes-file-all-flag-types

Conversation

@vidigoat

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • bug

What this PR does / why we need it:

TakesFile is a field of every flag type (FlagBase), and its only reader is the fish completion script, but fishAddFileFlag only checks *StringFlag and *StringSliceFlag. Any other flag with TakesFile: true still gets -f, so fish completes no files for it:

&cli.StringFlag{Name: "config", TakesFile: true},
&cli.GenericFlag{Name: "input", Value: &p, TakesFile: true},
&cli.StringMapFlag{Name: "env-file", TakesFile: true},
complete -c app -n '__fish_app_no_subcommand' -l config -r ...
complete -c app -n '__fish_app_no_subcommand' -f -l input -r ...
complete -c app -n '__fish_app_no_subcommand' -f -l env-file -r ...

In fish 4.9.3, complete -C "app --config " lists the files in the directory, while complete -C "app --input " lists nothing. v2's fishAddFileFlag also handled GenericFlag (and PathFlag, which v3 replaced with StringFlag + TakesFile).

FlagBase now has an unexported takesFile() (TakesFile and the flag takes a value), and fishAddFileFlag checks for it instead of the two concrete types. So every flag type works, there is no API change (make v3diff is clean), and a BoolFlag with TakesFile keeps -f, as it does now, since it has no value to complete.

Which issue(s) this PR fixes:

None, found while testing the generated fish script in fish.

Testing

  • New TestFishCompletionTakesFileForAllFlagTypes (StringFlag, GenericFlag, StringMapFlag, a plain StringFlag and a BoolFlag). It fails on main.
  • Checked the generated script in fish 4.9.3: --input and --env-file now complete files like --config.
  • make (generate, vet, tests, binary size, gfmrun), make lint and make v3diff pass. go test -race passes, as do the tests with urfave_cli_no_template, urfave_cli_no_docs and urfave_cli_no_suggest.

The change and the test were written with Claude Code (AI).

Release Notes

TakesFile now enables file completion in the fish script for every flag type, not only StringFlag and StringSliceFlag.

🤖 Generated with Claude Code

@vidigoat
vidigoat requested a review from a team as a code owner October 24, 2026 17:32
TakesFile is a field of every flag type, but fishAddFileFlag only
checked StringFlag and StringSliceFlag, so a GenericFlag or
StringMapFlag with TakesFile still got `-f` and fish offered no files
for it. v2 also handled GenericFlag. Flags now report it through an
unexported method on FlagBase; a flag that takes no value (BoolFlag)
keeps `-f`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vidigoat
vidigoat force-pushed the fix/fish-takes-file-all-flag-types branch from 304daa0 to 01c93c2 Compare October 25, 2026 11:56

@Juneezee Juneezee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Reproduced on main with fish 4.9.3: complete -C 'app --input ' (GenericFlag) and complete -C 'app --env-file ' (StringMapFlag) list nothing. With this PR both list files. The new test fails on main and passes here.

A bonus of the method on FlagBase: a user type that embeds a flag, such as type pathFlag struct{ cli.StringFlag }, now gets file completion too. The old type switch missed it.

Optional nit: this comment describes the old code, which readers won't see after the merge:

cli/fish.go

Line 164 in 01c93c2

// TakesFile is a field of every flag type, not only StringFlag and StringSliceFlag

Maybe say what -f does instead, e.g. // -f stops fish from completing file names for the flag's value. Fine to merge as is.

@Juneezee
Juneezee merged commit b9b90e4 into urfave:main Oct 25, 2026
9 checks passed
Juneezee pushed a commit that referenced this pull request Oct 25, 2026
The comment described the type switch that the takesFile method
replaced in #2500.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

2 participants