Auto enable completion

[wfxr]

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

We can enable completions automatically for zsh and bash.

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:

[wfxr]

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]

@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]

@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]

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]

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]

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]

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!