-
Notifications
You must be signed in to change notification settings - Fork 372
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
feat: generate completion subcommand #1561
feat: generate completion subcommand #1561
Conversation
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. |
This looks great @plustik! I'll try it out/review soon. |
@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. |
@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. |
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. |
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. 👍 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. |
I was able to fix most of your examples and while the long description for 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. @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. |
src/cli.rs
Outdated
@@ -595,7 +601,7 @@ pub struct Opt { | |||
/// affect delta's performance when entire files are added/removed. | |||
pub line_buffer_size: usize, | |||
|
|||
#[arg(long = "line-fill-method", value_name = "STRING")] | |||
#[arg(long = "line-fill-method", value_name = "STRING", value_parser = ["ansi spaces"])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a test failure I think because this should be split into two words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I pushed the fix)
Thanks very much @plustik, and @exploide and @maverick1872. I've merged this, with the auto-generated completions in the repo. |
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.