dandavison / delta

A syntax-highlighting pager for git, diff, grep, and blame output
https://dandavison.github.io/delta/
MIT License
21.43k stars 361 forks source link

feat: generate completion subcommand #1561

Closed plustik closed 8 months ago

plustik commented 8 months ago

Fixes #1167 #1022

As seen in issue 1167 and issue the existing completion script for ZSH does not work right now.

One solution discussed here is adding a subcommand to generate a completion file for ZSH/for all shells. I implemented this subcommand in these changes. The completion file generated by the new subcommand fixes both issues 1167 and 1022.

This solution enables us to generate up-to-date completion files for multiple shells including ZSH, Bash and Fish when needed. I consequently removed the existing completion files from etc/completion. If you prefer keeping these files I could also just replace them with the scripts generated with the new commands.

maverick1872 commented 8 months ago

I just updated #1167 with a basic completion that I finally got around to putting together that would solve the issue with it. That being said if they could be auto-generated that would be super nice for long term maintenance.

Might be valuable to include in the PR an example of what the generated completion files would look like if @dandavison were to accept it for ease of reviewing.

dandavison commented 8 months ago

This looks great @plustik! I'll try it out/review soon.

dandavison commented 8 months ago

@plustik thanks a lot for doing this -- would you be able to add some instructions to the manual?

This certainly looks like the right approach to me. I've tried the zsh completions and they seem to work correctly. I'd like to delete the existing completion files rather than distribute them. I guess this may break some downstream packaging scripts, but I still think it's the right thing to do, especially since delta is not at 1.x yet. Anyone, feel free to tell me otherwise.

maverick1872 commented 8 months ago

@dandavison the other alternative would be to generate the completions as a part of the release artifacts and distribute accordingly. Both would be appropriate in my eyes. It's a question of who has the onus to generate them.

Short term would be a question of retaining backwards compatibility.

Long term though I agree the completions should be generated at install time.

dandavison commented 8 months ago

Short term would be a question of retaining backwards compatibility. Long term though I agree the completions should be generated at install time.

Agreed. But I can only think of quite inconvenient ways of communicating the deprecation to downstream maintainers.

OK I've pushed to your branch @plustik: a Makefile target and dumped the output to the existing files.

exploide commented 8 months ago

As the person who contributed the fish completions in #1165 and @dandavison notified me there, I would like to leave my comment.

I like the approach of generating shell completions automatically when the CLI framework supports it. This helps keeping the completions up to date and results in less manual work in the end. :+1:

I recommend documenting this for package maintainers, so they are aware that they should generate the completion files before packaging delta. However, since you added that to the Makefile, it might be sufficient, I'm not sure.

One possible drawback to consider is that automatically generated completions might be worse than manual completions when it comes to descriptions and argument completion. This can usually be solved by augmenting the CLI definition with additional data when the generation framework supports that. I don't know clap and cannot say if this is the case here. Some examples:

I'm not saying this is a must have for the first attempt of generated completions, but I think it would be great if these could be added eventually. I know that some Go tools which use the Cobra framework, like podman or docker have awesome argument completion. I could imagine that clap is also capable of this, but I don't know how much work it is.

plustik commented 8 months ago

One possible drawback to consider is that automatically generated completions might be worse than manual completions when it comes to descriptions and argument completion. This can usually be solved by augmenting the CLI definition with additional data when the generation framework supports that. I don't know clap and cannot say if this is the case here. Some examples:

* The description for `--grep-output-type` is ridiculously long. This is not helpful as it gets truncated anyway.

* Argument completion is currently only present for `--generate-completion` itself, but not for other options. In the manual completion script for fish, there are at least completions for:

  * `--inspect-raw-lines` with values: "true false"
  * `--line-fill-method` with values: "ansi spaces"
  * `--paging` with values "auto always never"
  * `--true-color` with values "auto always never"
  * `--syntax-theme` automatically generated with "(delta --list-syntax-themes | cut -f 2)"

I'm not saying this is a must have for the first attempt of generated completions, but I think it would be great if these could be added eventually. I know that some Go tools which use the Cobra framework, like podman or docker have awesome argument completion. I could imagine that clap is also capable of this, but I don't know how much work it is.

I was able to fix most of your examples and while the long description for --grep-output-type isn't perfect it's not a big drawback from my point of view. But I don't think there is a good solution for completing --syntax-theme without editing the completion files after generation.

My point is: The automatic generation can definitely be improved further by adding more information to the CLI definition, but don't think clap allows us to generate completion files as good as hand-written files can be. In the end it's a decision between maintaining perfect completion files by updating them manually after every change to the CLI and generating good enough files automatically.

@dandavison I added an entry to the manual. I'm not sure, whether I included all relevant information. If you'd expect anything else in that page, I can add that. I think there are cleaner solutions for the argument completions I fixed in #e516736, but they would require changing the types of the corresponding attributes in cli::Opt, which would mess with the set_options macro. I don't know much about the codebase, but I don't think it's worth the effort.

dandavison commented 8 months ago

Thanks very much @plustik, and @exploide and @maverick1872. I've merged this, with the auto-generated completions in the repo.