wfxr / forgit

:zzz: A utility tool powered by fzf for using git interactively.
MIT License
4.33k stars 136 forks source link

Add bash completions for forgit functions and aliases #235

Closed carlfriedrich closed 1 year ago

carlfriedrich commented 1 year ago

This is my shot for porting @cjappl 's changes of #231 to bash. Not every forgit function supports arguments at the moment, but at least for the ones that support it completion is a useful thing, so I added it for all functions at once, just like in fish.

@wfxr What do you think? And do you know how this could be implemented for zsh?

Tested all completions in bash.

Check list

Description

Type of change

Test environment

cjappl commented 1 year ago

Cool! I had no idea that bash supported such a thing 👀

wfxr commented 1 year ago

@wfxr What do you think? And do you know how this could be implemented for zsh?

@carlfriedrich Good idea! I'm not familiar with this but you can refer to fzf: https://github.com/junegunn/fzf/tree/master/shell

There are two suggestions:

  1. Complete the original functions instead of the aliases?
  2. Move the completion code into a separate file like many other tools to avoid code bloat.
carlfriedrich commented 1 year ago

@wfxr Thanks for your feedback.

1. Complete the original functions instead of the aliases?

Are there actually people who use the functions directly? Or does zsh complete aliases automatically equally to the commands they alias to? Bash at least doesn't.

2. Move the completion code into a separate file like many other tools to avoid code bloat.

Is this compatible with the oh-my-zsh plugin system? Where should these completions go within the forgit tree then?

Since we're mainly re-using existing git completions, I don't think the code gets bloated too much. And the fish code includes the completions directly in the code as well. But I'm open for discussion here.

wfxr commented 1 year ago

@carlfriedrich Thank you. Serveral lines don't make the code bloated but many times of serveral lines will do. I prefer to keep the single main file short and efficient. Not just for running efficiently, but also loading efficiently, and reading efficiently.

Are there actually people who use the functions directly? Or does zsh complete aliases automatically equally to the commands they alias to? Bash at least doesn't.

Someone use custom aliases instead the default. zsh does support complete aliases the same as it's original command.

Is this compatible with the oh-my-zsh plugin system?

IDK but should not a problem since there are many pure completion plugins in omz. Also most plug managers support to specify the completion file path when loading the plugin.

Where should these completions go within the forgit tree then?

A new directory in root named completions is ok for me.

carlfriedrich commented 1 year ago

@carlfriedrich Thank you. Serveral lines don't make the code bloated but many times of serveral lines will do. I prefer to keep the single main file short and efficient. Not just for running efficiently, but also loading efficiently, and reading efficiently.

@wfxr OK, I get your point here.

Are there actually people who use the functions directly? Or does zsh complete aliases automatically equally to the commands they alias to? Bash at least doesn't.

Someone use custom aliases instead the default. zsh does support complete aliases the same as it's original command.

Ah, good to know. I will consider this.

Where should these completions go within the forgit tree then?

A new directory in root named completions is ok for me.

Alright, I will transfer the completions accordingly.

carlfriedrich commented 1 year ago

@wfxr So here's the updated PR with my attempt to provide a separate completion file for bash. It completes the following variants of calling forgit:

  1. forgit functions (e.g. forgit::add)
  2. shell aliases (e.g. ga)
  3. git-forgit wrapper calls (e.g. git forgit add)
  4. git-forgit aliases (e.g. git a with alias.a=forgit add in git config)

The only change left in the main forgit file is the export of all the alias definitions, because that's the only way of knowing these in the completion file. Alternative would be to redefine them over there, but then we would have hard-coded them twice, which bears the risk of running out of sync.

What do you say?

Should we include the completion script in our shellcheck test?

And should/can we implement a similar solution for zsh?

carlfriedrich commented 1 year ago

@wfxr I updated this PR to the current master. Do you mind if I merge this? It adds completions for bash in a dedicated file. I have actually been using these for several weeks now, so they are proven working. The patch should not interfere with zsh or fish environments.

carlfriedrich commented 1 year ago

I decied to adapt this PR to support the following two completions only:

These are my only actual use cases and they don't require any changes in the code, so I consider this PR minimal obstrusive and will merge it directly now.

For the plugin functions/aliases completion I will create another PR.