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

feat: generate completion subcommand #1561

Merged
merged 7 commits into from Nov 19, 2023
Merged

feat: generate completion subcommand #1561

merged 7 commits into from Nov 19, 2023

Conversation

plustik
Copy link
Contributor

@plustik plustik commented Nov 10, 2023

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
Copy link

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
Copy link
Owner

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

@dandavison
Copy link
Owner

@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
Copy link

@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
Copy link
Owner

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
Copy link
Contributor

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:

  • 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.

@plustik
Copy link
Contributor Author

plustik commented Nov 18, 2023

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.

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"])]
Copy link
Owner

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.

Copy link
Owner

Choose a reason for hiding this comment

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

(I pushed the fix)

@dandavison dandavison merged commit 49a9918 into dandavison:master Nov 19, 2023
9 of 12 checks passed
@dandavison
Copy link
Owner

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

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.

🚀 [Question] no autocompletion for second path
4 participants