Skip to content

fix: make fish completions work #2096

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: make fish completions work #2096

wants to merge 2 commits into from

Conversation

yjp20
Copy link

@yjp20 yjp20 commented Apr 17, 2025

What type of PR is this?

  • bug

What this PR does / why we need it:

This PR fixes a bug where ./cli completion fish always outputs

# completion fish shell completion

function __fish_completion_no_subcommand --description 'Test if there has been any subcommand yet'
    for i in (commandline -opc)
        if contains -- $i
            return 1
        end
    end
    return 0
end

complete -c completion -n '__fish_completion_no_subcommand' -f -l help -s h -d 'show help'
complete -c completion -n '__fish_completion_no_subcommand' -f -l help -s h -d 'show help'

This is because the command used to generate the completions is always the one corresponding to the completion command, and it needs to know the root command.

Which issue(s) this PR fixes:

issue #2105

Testing

After making these changes, completions for my CLI generated successfully.

Release Notes

Fixes issue with fish completions which showed completions only for the `complete` command

@yjp20 yjp20 requested a review from a team as a code owner April 17, 2025 09:47
@dearchap
Copy link
Contributor

@yjp20 You dont need to make all these changes you could have done the following

https://github.com/urfave/cli/blob/main/command_setup.go#L92

completionCommand := buildCompletionCommand(cmd.Root().Name)

@yjp20
Copy link
Author

yjp20 commented Apr 21, 2025

You dont need to make all these changes you could have done the following
completionCommand := buildCompletionCommand(cmd.Root().Name)

I'm pretty sure this isn't sufficient, because you have to do c.ToFishCompletion() as a part of fish handler, so c here definitely has to be the rootCmd. You can check this out yourself if you'd like.

If you're averse to code changes, I could also change this such that the fish handler does c.Root().ToFishCompletion(), but frankly this feels a bit weirder to me, though I'm happy to make this change.

@dearchap
Copy link
Contributor

Will check and get back to you

@dearchap
Copy link
Contributor

Fixed by #2106

@dearchap dearchap closed this Apr 21, 2025
@dearchap dearchap reopened this Apr 21, 2025
@yjp20 yjp20 closed this Apr 21, 2025
@yjp20 yjp20 reopened this Apr 21, 2025
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.

3 participants