-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
auto-completions for bb.cli/dispatch #95
base: main
Are you sure you want to change the base?
Conversation
@Sohalt Trying to test:
I'm getting:
|
96e1f36
to
d030850
Compare
The installation for zsh can vary a bit, depending on how your distro packages zsh. |
Also I just rebased on main and force pushed a new version of |
What is the recommended approach for this Rust-based CLI package? I suppose they must have some user-friendly docs for this? |
They use |
What definitely should work is this:
|
Tests are failing due to line endings on Windows. Does any of bash, zsh or fish even work on Windows? What's the situation there when using something like Cygwin? |
Current limitations:
|
We might want to add Powershell completions or so in the future, but let's not worry about that for now. Does the example Rust library support this? You can disable those tests for Windows |
no afaics
how? |
By wrapping the whole test body in:
|
Sometimes things can be so simple :) I thought there was some yaml programming involved to configure the test runner or similar 😅 |
5d9df89
to
38f5c16
Compare
"--babashka.cli/completion-snippet" | ||
(if-let [command-name (get-in opts [:completion :command])] | ||
(do (print-completion-shell-snippet (keyword shell) command-name) | ||
(System/exit 0)) |
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 prefer not to call System/exit in function that are supposed to be non-side-effecting. I'd rather throw exceptions or whatever
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.
We need to short circuit all following execution in this case though, since we're printing the shell completion snippet and any other code running after this might print stuff that messes this up. It only happens when you pass the internal --babashka.cli/completion-snippet
opt, so regular user code should never hit this.
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.
You don't need to short circuit if you just put the logic after all the parsing logic though?
Also removing the System/exit stuff makes it more REPL friendly
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.
If I put the code after the parsing logic the consuming code could still print extra stuff. e.g.
(defn -main [& args]
(cli/parse-opts args opts)
(println "this does not belong in the completions script"))
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.
The alternative would be to make it the responsibility of the user to call print-completion-shell-snippet
(and make sure nothing else is printed in that case)
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.
Well, we control format-opts
so we can decide to filter those options out, right?
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.
Sure, but then you'll have a format-opts
, that magically swallows some options.
I also don't particularly like the magic in parse-opts
, but at least that means you get completions without any setup on your part.
Changing format-opts
means you still have magic and need to do setup.
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'm also ok with just providing the complete
* functions and having the user wire everything up themselves. Then it's even more work, but absolutely no magic.
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.
Well, we control
format-opts
so we can decide to filter those options out, right?
Or do you mean the caller filters them out?
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.
magically swallows some options
The fact that org.babashka.cli/...
options are internal, makes this non-magic imo. Users of this library should never make up options with that namespace themselves
(System/exit 0)) | ||
(binding [*out* *err*] | ||
(println "Need `:completion {:command \"<name>\"}` in opts to support shell completions") | ||
(System/exit 1))) |
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.
Same here
ef3539e
to
3d1a9ed
Compare
3d1a9ed
to
00e4a2a
Compare
You can test it with