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 bash completion #1678

Merged
merged 4 commits into from
Jun 28, 2021
Merged

Add bash completion #1678

merged 4 commits into from
Jun 28, 2021

Conversation

scop
Copy link
Contributor

@scop scop commented Jun 3, 2021

Closes #1010

This implementation is self contained and does not require https://github.com/scop/bash-completion. If a dependency on it would be ok, adding it would make it possible to clean this up some, and would gain easy support for completing long options separated by = (which this one doesn't currently, and I'd rather not duplicate the code for that). Let me know if that would be desirable and I can adjust this.

@sharkdp sharkdp closed this Jun 10, 2021
@sharkdp
Copy link
Owner

sharkdp commented Jun 10, 2021

Thank you very much for your contribution!

I am not familiar with shell completion scripts myself. Maybe @eth-p?

(sorry for the accidental close)

@sharkdp sharkdp reopened this Jun 10, 2021
@eth-p
Copy link
Collaborator

eth-p commented Jun 12, 2021

@sharkdp I'm not too familiar with Bash completions, but I'll give it a look. I'll need to give it a proper review later, but this seems good from a quick glance.

This implementation is self contained and does not require https://github.com/scop/bash-completion. If a dependency on it would be ok, adding it would make it possible to clean this up some, and would gain easy support for completing long options separated by = (which this one doesn't currently, and I'd rather not duplicate the code for that). Let me know if that would be desirable and I can adjust this.

I would actually say this is one of those cases where it's fine to add an external dependency. From my understanding, a ton of other programs use bash-completion for their completions as well, and there's probably a good chance it will be installed already.

In the cases where it isn't (e.g. Mac), it shouldn't be too hard for package managers to simply add a dependency.

@Shark, thoughts?

@scop
Copy link
Contributor Author

scop commented Jun 13, 2021

FWIW I do support adding the dependency, but I may also be quite heavily biased. In any case, the changes that I'd make if the dependency would be desirable are quite small and easy to review, so if the decision is not immediately clear, starting to review this in its current shape wouldn't be throwaway work if the dep+changes were introuced later.

@sharkdp
Copy link
Owner

sharkdp commented Jun 14, 2021

Adding that dependency sounds good to me if it helps with the implementation.

scop added 2 commits June 14, 2021 17:20
For = option/arg separator support, improved mid-word completion
behavior, code cleanliness.
@scop
Copy link
Contributor Author

scop commented Jun 14, 2021

Ok, implementation adjusted to depend on it.

@sharkdp
Copy link
Owner

sharkdp commented Jun 14, 2021

Thank you very much. This looks good to me.

I think this would be worth an entry in the CHANGELOG.md file. It would be great if you could add one (https://github.com/sharkdp/bat/blob/master/CONTRIBUTING.md#add-an-entry-to-the-changelog), otherwise I can also do it myself.

@scop
Copy link
Contributor Author

scop commented Jun 14, 2021

Sure, added to the Other section.

@ds2606
Copy link

ds2606 commented Jun 22, 2021

Just commented in #1010 that completions seem to re-surface the original issues for me (generic fliename completions are failing in zsh for bat) so I'll need to manually remove the completion scripts for the time being. The other completions are nice, but my primary use case for bat is just the default command with a completion filename (often in some deeply nested directory), so filename completion functionality is the most important for me. Could we perhaps add a command to disable completion files?

@scop
Copy link
Contributor Author

scop commented Jun 28, 2021

For completeness, noting here too: as refined and discussed in #1010, this particular completion does not cause the above mentioned breakage to occur in bash.

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you!

@sharkdp sharkdp merged commit 0e9d612 into sharkdp:master Jun 28, 2021
@scop scop deleted the feat/bash-completion branch June 28, 2021 10:13
@eggbean
Copy link

eggbean commented Jul 14, 2021

Not a big problem, but curious to know why I cannot get this to work.

I added bat.bash.in to etc/bash_completion.d/ (actually a stow symlink, as that is how I install bat) and sourced ~/.profile but I get no indication anything has changed. This way works for several other programs I install using stow, so I suspect the unusual .in suffix has something to do with it?

Also, the file does not seem to be included in the latest .deb packages.

@sharkdp
Copy link
Owner

sharkdp commented Jul 14, 2021

I added bat.bash.in to etc/bash_completion.d/

Ah, that won't work. bat.bash.in is just a template for a bash completion file (hence the .in suffix). The actual bat.bash completion file will be generated when you build the project with cargo. Alternatively, download one of the tarballs / Debian packages from the release page. They also contain the generated files.

@sharkdp
Copy link
Owner

sharkdp commented Jul 14, 2021

Oh - they're actually not part of the Debian packages (but the tarballs). That should be fixed in .github/workflows/CICD.yml => #1733.

@michaelblyons
Copy link
Contributor

Is this also missing from Homebrew?

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.

Bash CLI completion script?
6 participants