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

feat(completions): add completion for fish #344

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

folliehiyuki
Copy link
Contributor

fish's builtin git completion automatically registers git-forgit completions as completions for forgit subcommand of git. Therefore this PR provides completions for both formats git-forgit and git forgit.

Simple subcommands get the completion list from __fish_git_* functions, while others requiring more than 1 __fish_git_* completion sources reuse the completion items from the corresponding git commands.

Check list

  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation

Description

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation change

Test environment

  • Shell
    • bash
    • zsh
    • fish
  • OS
    • Linux
    • Mac OS X
    • Windows
    • Others:

fish's builtin git completion automatically registers git-forgit
completions as completions for forgit subcommand of git.
@cjappl cjappl self-requested a review February 5, 2024 19:40
@cjappl
Copy link
Collaborator

cjappl commented Feb 5, 2024

Hi @folliehiyuki!

Thanks so much for the contribution(s) 🎉

Could you please:

  1. Figure out why the ci is faililng
  2. Give me a few test cases I can run locally on my machine with explicit detail to ensure the feature is working?

I'm the main fish maintainer, so I want to be sure I understand the feature fully and do some QA before merging!

@sandr01d
Copy link
Collaborator

sandr01d commented Feb 5, 2024

I ran into the same issue with the CI previously, but the issue went away by itself before I could do anything to fix it. For some reason the fish PPA fails to install

Cannot add PPA: 'ppa:~fish-shell/ubuntu/release-3'.
ERROR: '~fish-shell' user or team does not exist.
Error: Process completed with exit code 1.

I'm suspecting an issue with the fish PPA. This is not caused by the code changes, the CI fails before the actual checks.

Just ran the test locally on my machine and they appear to be fine again. Could not test macos though, because act does not support this.

@folliehiyuki
Copy link
Contributor Author

folliehiyuki commented Feb 6, 2024

Give me a few test cases I can run locally on my machine with explicit detail to ensure the feature is working?

Just use git-forgit and git forgit as you normally do (while spamming the <TAB> key)!

mkdir test-repo && cd test-repo
git init
touch file{1,2,3}

# See that the completion list includes all subcommands
git forgit <Tab>
git-forgit <Tab>

# Completion for `add`
git-forgit add <Tab><Tab>
git forgit add <Tab><Tab>
...

All the __fish_git_* functions are sourced from the completion file of git, so you need it in-place first. It comes by default in every fish-shell package.

@cjappl
Copy link
Collaborator

cjappl commented Feb 6, 2024

@folliehiyuki this is great! Working as you intended.

A couple questions:

  1. Do you have any thoughts on if this should live in it's own file, or these methods be run at the end of forgit.plugin.fish? I don't know the overhead or cost, but I think it would be nice for the fish completions to "just work" when someone sources forgit. I am completely new in the area of fish completions, so I'm interested in your past knowledge or gut feeling on this.
  2. I'm noticing these completions are now much nicer than the ones on the normal functions/aliases (glo <TAB is very sad in comparision ) if you're feeling up to it, after this gets submitted one of us should go clean up those completions as well! This is a great upgrade.

Let me know what you think on point 1. Depending on that we can submit.

@folliehiyuki
Copy link
Contributor Author

folliehiyuki commented Feb 6, 2024

On point 1, see https://fishshell.com/docs/current/completions.html#where-to-put-completions.

  • Fish plugin managers like oh-my-fish and fisher are smart enough to put the completion into the right place in your $__fish_config_dir/completions on installation (see https://github.com/jorgebucaran/replay.fish for an example of the repo structure the plugin managers understand, which we already follow nicely).
  • Furthermore, fish completion files are lazy-loaded (which means they're sourced the 1st time you press <TAB> on the corresponding command).

So, it's better to put the fish completion in a separated file. It works with just the git-forgit command inside $PATH (I actually use git-forgit this way without all the aliases). There is no need to source the fish plugin.

On point 2, as said above, I don't use the fish plugin personally, so it's hard to judge. My recommendation would be to use abbr instead of aliases, so, on the command line, pressing glo<Space> would actually result to git forgit log being displayed and we'll have nice completion (still with fewer key presses).

@cjappl
Copy link
Collaborator

cjappl commented Feb 6, 2024

Awesome. Makes sense to me. Thanks again for the contribution @folliehiyuki ! merging it now.

If you feel you have energy to tune up the completions in the non-git-plugin version, another contribution would be greatly appreciated, otherwise I'll check it out when I have time.

Cheers!

@cjappl cjappl merged commit cf447a6 into wfxr:master Feb 6, 2024
2 of 4 checks passed
cjappl pushed a commit that referenced this pull request Mar 5, 2024
Switch from aliases to abbreviations, as I suggested in

    feat(completions): add completion for fish #344 (comment)
    string collect returns 1 on empty argument, so it's used here to avoid repeating if else end blocks. Another benefit is that the output of string collect is ensured to be a single string. From string --help:

    string collect collects its input into a single output argument, without splitting the output when used in a command substitution. This is useful when trying to collect multiline output from another
    command into a variable. Exit status: 0 if any output argument is non-empty, or 1 otherwise.

This PR is marked as breaking change, since abbr behaves differently from alias. It can only be used in the interactive command line (so putting exec glo into your scripts won't work).
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.

None yet

3 participants