search

wfxr / forgit

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

Auto enable completion #346

Closed wfxr closed 4 months ago

wfxr commented 4 months ago

Check list

Description

We can enable completions automatically for zsh and bash.

Type of change

Test environment

wfxr commented 4 months ago

This pr works for me, but fails in ci because of the lack of _git. And considering the reason @sandr01d explained in #310:

The reason for this is that zsh's autoload does not load the files on shell start, but dynamically the first time a completion is needed. Having the compdef commands that bind the completions for the shell plugin in the main file would lead to the completions becoming available after git forgit has been completed for the first time. Because this behavior would be inconsistent and confusing, I've placed them in a separate file that has to be sourced explicitly.

I think it's better to sourcing the completions/git-forgit.zsh manually.

carlfriedrich commented 4 months ago

@wfxr I actually like this idea and it makes perfect sense to me. @sandr01d @cjappl What do you think?

In order to fix the shellcheck IMO we would have to install the git completions before the tests, or use a container which already has them. I think this shouldn't be too difficult, or am I wrong?

wfxr commented 4 months ago

@wfxr I actually like this idea and it makes perfect sense to me. @sandr01d @cjappl What do you think?

In order to fix the shellcheck IMO we would have to install the git completions before the tests, or use a container which already has them. I think this shouldn't be too difficult, or am I wrong?

@carlfriedrich Thanks for the quickly testing! I'm not sure how to avoid the _git not found error gracefully if someone actually doesn't have git completions installed. @sandr01d Is it possible to delay this error until completion is triggered instead of when completions/git-forgit.zsh is sourced?

carlfriedrich commented 4 months ago

Now that I look at it again I think this should actually be fixed in the ZSH completion file. We rely on the git completions being installed, while actually we should have error handling for that in case they are not. @sandr01d Can you take a look if I see that correctly? I am gonna reopen this for discussion.

sandr01d commented 4 months ago

Only saw this PR today, sorry for the late reply. I think that the issue this PR is trying to solve has already been solved by #340, where @cjappl found a way to auto enable zsh completions whithout having to source any additional files (be it manually or automatically).

Now that I look at it again I think this should actually be fixed in the ZSH completion file. We rely on the git completions being installed https://github.com/wfxr/forgit/blob/master/completions/_git-forgit#L97, while actually we should have error handling for that in case they are not.

We currently do rely on the git completions being present and simply fail with an exit code of 1 when a user tries to use a completion and they aren't. I guess in this case we should simply print out a user facing error message letting the user now whats going on. _message appears to be the correct utility function to do so. @carlfriedrich Is that what you have in mind?

cjappl commented 4 months ago

Yeah, I think we should re-test with the newly merged file and see if there is any difference in what we intend to be the user experience vs how we have it now.

I think the latest version is pretty streamlined and also follows the "best practices" i saw in many other zsh completions. The main thing I'd worry about with this PR is actively loading/sourcing the completions on shell startup instead of lazy loading when the completion is requested.

TLDR: check out main with the new completion file in your fpath and see if that leaves anything unaccounted for :)

carlfriedrich commented 4 months ago

I guess in this case we should simply print out a user facing error message letting the user now whats going on. _message appears to be the correct utility function to do so. @carlfriedrich Is that what you have in mind?

That sounds reasonable.

However, if #340 already provided a better implementation for this kind of problem, I'm gonna close this again. Thanks for your comments!