-
-
Notifications
You must be signed in to change notification settings - Fork 18
[#133] Added shell completion #134
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
Conversation
@MitchellBerend Just to have TL;DR: does it mean that completion doesn't work if the binary has the name |
It does, the way it is implemented in the pr right now it gets the name automatically. There is an easy way to override this with |
@MitchellBerend That's great to hear! I see that this PR is still draft. Would you prefer to wait for the |
Yes I would, I put a blocked tag on the issue itself. I don't think i can put labels on prs but consider this one blocked until #136 is resolved. |
@MitchellBerend Sorry to have kept you waiting, I've opened #137 for the |
@SanchithHegde don't worry about it, Im at a convention anyway 😅. |
There is an issue with the help text of the current There is a config option ( Im not sure if I like the way the formatting looks when the indentation are removed. I added ` around the commands to make it a little clearer but this still looks kind of like a wall of text to me. There is an issue filed on the main rust repo for this already. |
Sample output:
|
@MitchellBerend Thanks for writing such detailed progress updates! The issue with doctest is annoying indeed. The solution with backticks sounds like a good trade-off. And thanks a lot for write great help text! 💯 |
Sample output of current implementation
|
…ed some text markers
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.
This looks super great! 🏆
I have a few suggestions on refactoring, documenting and changing the module structure a bit. But the implementation is superb ✨
src/config/template.rs
Outdated
@@ -44,3 +46,46 @@ fn config_template() -> String { | |||
version = env!("CARGO_PKG_VERSION"), | |||
) | |||
} | |||
|
|||
// This function can break when clap_complete adds support for a new shell type |
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.
Will it break though? I see that the Shell
type has the #[non_exhaustive]
pragma. And we also handle all other shells with _
.
So, in other words, the compiler won't output any errors if clap_complete
adds new shells. They'll simply remain unsupported as now. And people can open new issues to request support for new shells 🙂
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 added that comment to clarify why there is a separate function for a simple match statement. Maybe this should include an error message along the lines of:
This shell type is not implemented yet, please go over to https://github.com/chshersh/tool-sync to file an issue about its implementation.
Co-authored-by: Dmitrii Kovanikov <kovanikov@gmail.com>
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.
Looks amazing! LFG 🚀
Resolves #133
This pr aims to add a tab completion script so users don't have to remember every command if they want to experiment with config options.
Additional tasks