-
Notifications
You must be signed in to change notification settings - Fork 238
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
Fix shell completion scripts #501
Fix shell completion scripts #501
Conversation
No detail for those yet... We also make sure they're in the same order as what's output by "usage: "
No detail for those yet... We also make sure they're in the same order as what's output by "usage: ", and using the same text
All commands implemented A smarter(?) way to detect executing command More restrictions on input (all those if) Less surprises on reading the code (more variables)
Looks good. We should merge this when you're ready |
Yeah, I'm gonna right the Zsh ones first, then we can merge 😄 |
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.
Probably best seen with Hide whitespace
enabled...
_kerl_available_releases() { | ||
# we use the file directly as `kerl list releases` dumps extraneous data to stdout | ||
releases=(${(f)"$(_call_program releases cat ~/.kerl/otp_releases)"}) | ||
_kerl_otp_releases() { |
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.
Functions were renamed to approach file names...
} | ||
|
||
_kerl_builds() { | ||
builds=(${(f)"$(_call_program builds kerl list builds 2>/dev/null | cut -f 2 -d ",")"}) |
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 to rely on file contents, then have to eventually deal with side-effects these function calls might have.
_kerl_available_releases | ||
build) | ||
_arguments \ | ||
'1: :->release' \ |
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.
Variables were renamed to approach the actual documentation.
elif [[ "$state" == buildnames ]]; then | ||
|
||
fi;; |
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.
Was empty, so was removed.
All commands implemented More restrictions on input: fixes all found issues Less surprises on reading the code (variable naming) Whitespace tweaked
I pushed the Zsh completion. I'm re-requesting your review in case something obvious pops up (or even non-obvious fwiw). Thanks. |
@jadeallenx, shall I merge as-is, or do you wanna review fda9b87? |
Sorry - yeah we should merge this, thanks! |
Description
Updates on the shell completion scripts, for Bash and Zsh.
Opening with Bash first (as draft), for early feedback and self-review purposes, before moving on to Zsh.
For review purposes, I believe a commit-by-commit makes it easier.
For checking out the final result I'd propose cloning it locally and playing with it; I'm not sure there's an easy way to test shell completion scripts, but I did my best to test it manually.
Closes #273.