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

Add zsh completion #1136

Merged
merged 8 commits into from
Sep 14, 2020
Merged

Add zsh completion #1136

merged 8 commits into from
Sep 14, 2020

Conversation

Kienyew
Copy link
Contributor

@Kienyew Kienyew commented Aug 8, 2020

No description provided.

@@ -0,0 +1,56 @@
#compdef bat
# FIXME: help me with the subcommand `cache`, zsh completion is hard as hell
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never written any completion scripts. Let's leave this open until someone can help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'll dive into the zsh manual again later, sorry to say but is one of the most difficult manual I have read IMO 🤣

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be also okay with merging this without support for the cache subcommand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the completion of cache subcommand in 32905f7

@sharkdp
Copy link
Owner

sharkdp commented Aug 15, 2020

Thank you very much for your contribution!

@sharkdp
Copy link
Owner

sharkdp commented Aug 30, 2020

Thank you very much for the updates. We should also make sure that the zsh completions is properly included in the tarballs and the Debian packages (see similar code for fish completions).

Also, could you please add an entry to the "unreleased" section in CHANGELOG.md? The format for entries is:

- Description what has been changed, see #123 (@user)

where #123 links to the bug ticket and/or the PR and user is your username.

@Kienyew
Copy link
Contributor Author

Kienyew commented Sep 4, 2020

... We should also make sure that the zsh completions is properly included in the tarballs and the Debian packages (see similar code for fish completions).

@sharkdp is the change suppose to be done in .github/workflows/CICD.yml? I don't have any experience in packaging and deploying as well as github action. After some searching I think the zsh completion script should be copy to /usr/share/zsh/site-functions/_bat when installing.

Kienyew added a commit to Kienyew/bat that referenced this pull request Sep 4, 2020
@Kienyew
Copy link
Contributor Author

Kienyew commented Sep 4, 2020

sorry and please ignore the 05bd691 commit up there, I messed up with some history stuff.

I did a few changes. Tell me if I did something wrong.

@sharkdp
Copy link
Owner

sharkdp commented Sep 7, 2020

Thank you for the updates. As you can see in the failed tests, the packaging stage currently fails with:

cp: cannot stat 'target/XXX/release/build/bat-*/out/assets/completions/bat.zsh': No such file or directory

@Kienyew
Copy link
Contributor Author

Kienyew commented Sep 8, 2020

Thank you for the updates. As you can see in the failed tests, the packaging stage currently fails with:

Thanks for the reply, I hope the things go without problem now.

@sharkdp sharkdp merged commit a86f3e5 into sharkdp:master Sep 14, 2020
@sharkdp
Copy link
Owner

sharkdp commented Sep 14, 2020

Thank you for the updates!

@sharkdp
Copy link
Owner

sharkdp commented Sep 14, 2020

I tried copying the generated file to /usr/share/zsh/site-functions/_bat and restarted zsh, but I didn't get any auto-completion for bat. How did you test this?

@Kienyew
Copy link
Contributor Author

Kienyew commented Sep 14, 2020

I tried copying the generated file to /usr/share/zsh/site-functions/_bat and restarted zsh, but I didn't get any auto-completion for bat. How did you test this?

zsh do the searches in the fpath array variable for various functions, including completion functions.

As of my installation of zsh from manjaro's repository, which is derivative of arch linux's, /usr/share/zsh/site-functions is included in fpath by default. But after I saw your comment, I then try to install zsh in a ubuntu container, and found that /usr/share/zsh/site-functions is not defaultly included in fpath, which means that each distributions and versions of zsh might have differences in fpath initially.

But what I found from https://github.com/zsh-users/zsh/blob/04bd9a44a74683ad0d83921bfb3aa0c4d5992c75/INSTALL#L254-L262 :

Writing third-party autoloadable functions
------------------------------------------

Third-party autoloadable functions, including but not limited to completion
functions, should be installed into the share/zsh/site-functions/ directory
under the respective installation prefix.  That would typically be written as
$(DESTDIR)$(PREFIX)/share/zsh/site-functions/ in a makefile.  If the
third-party tool's $(PREFIX) is not the same as zsh's prefix, then that
directory should be added to $fpath in zsh's initialization files.

(that is really a mess...)

After all, if the release is a deb package, according to this answer, I believe we should just change the /usr/share/zsh/site-functions/ to /usr/share/zsh/vendor-completions

@sharkdp
Copy link
Owner

sharkdp commented Sep 20, 2020

As of my installation of zsh from manjaro's repository, which is derivative of arch linux's, /usr/share/zsh/site-functions is included in fpath by default.

Hm, weird. I actually tested on Arch Linux. I can't get the completions working by putting _bat there. It also doesn't work in /usr/share/zsh/vendor-completions.

After all, if the release is a deb package, according to this answer, I believe we should just change the /usr/share/zsh/site-functions/ to /usr/share/zsh/vendor-completions

👍

That's what we do for fd as well: https://github.com/sharkdp/fd/blob/74c3431a2bf74ce78c26974f3cc07570e1d3baf3/ci/before_deploy.bash#L148

@sharkdp
Copy link
Owner

sharkdp commented Oct 2, 2020

Just now I had a chance to finally try out the zsh completions myself. They are absolutely fantastic!! Thank you very much for putting so much work into this.

@sharkdp
Copy link
Owner

sharkdp commented Oct 2, 2020

also... they have been relased in v0.16.0.

@mjturner
Copy link

@Kienyew This is very neat!

Perhaps I've missed it, but it wasn't obvious to me that the completion file had to be renamed to _bat to work. Not so much an issue for those installing distro packages but I'm using the binary on macOS so had to do the renaming myself. Happy to send a PR for an update to the README (perhaps a section under "Customization"?).

@Kienyew
Copy link
Contributor Author

Kienyew commented Oct 12, 2020

@mjturner

Perhaps I've missed it, but it wasn't obvious to me that the completion file had to be renamed to _bat to work. Not so much an issue for those installing distro packages but I'm using the binary on macOS so had to do the renaming myself.

I agree with you, it really needs some efforts to search up something about this kind of stuff.

Happy to send a PR for an update to the README (perhaps a section under "Customization"?).

How about we just tag one of the maintiner and discuss with them?

@mjturner
Copy link

mjturner commented Oct 13, 2020

How about we just tag one of the maintiner and discuss with them?

Sounds good.

@sharkdp Your thoughts? Would it make sense to add a "Shell completion" section to the README or would a README in assets/completions make more sense (the latter being included in autocomplete/ when a release is built).

@sharkdp
Copy link
Owner

sharkdp commented Oct 13, 2020

I think we could add such a section to the README, yes 👍

But before that, shouldn't we simply change the name of the file to _bat?

@mjturner
Copy link

Thanks for confirming - if no-one beats me to it I'll try and submit a PR later today.

I think the confusing part is that _bat doesn't obviously make it ZSH-specific. Perhaps the completion files could go in shell-specific directories rather than just being in assets/completions? There could be assets/completions/{fish,zsh} - or is that too much clutter as each would only have a single file?

@sharkdp
Copy link
Owner

sharkdp commented Oct 14, 2020

I think the confusing part is that _bat doesn't obviously make it ZSH-specific. Perhaps the completion files could go in shell-specific directories rather than just being in assets/completions? There could be assets/completions/{fish,zsh}

sounds good!

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