-
Notifications
You must be signed in to change notification settings - Fork 12
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
Added bash completion and fzf support #33
base: master
Are you sure you want to change the base?
Conversation
I think we should consider a For now bash completion and fzf will make most interactions with |
scripts/dvm
Outdated
} | ||
|
||
# Setup bash completion | ||
complete -F _dvm_completions dvm |
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 should probably guard this behind a conditional so as not to cause errors for anyone using other shells. Maybe something like:
if [[ "$(basename $SHELL)" == "bash" ]]; then
complete -F _dvm_completions dvm
fi
Alternatively, this could be a case statement, and I could add zsh support when I get a moment. :)
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.
Actually, come to think of it, another alternative would be to break out a separate completions file; that would allow us to keep the shell-specific code separate.
e.g. https://github.com/scop/bash-completion/tree/master/completions
and
https://github.com/zsh-users/zsh-completions/tree/master/src
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.
That's a good idea! :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.
Note: stackoverflow suggested using BASH_VERSION
instead of $SHELL
because $SHELL
is the users default shell, not necessarily the shell currently running.
Something like this? 😉
I'd actually forgotten this feature existed until a couple weeks ago. #12. Perhaps the bug was to actually create an alias symlink, rather than writing the file to be sourced which IIRC is what it does now. That's probably actually still a good idea. |
Oh I see, I misread what you'd written; you actually included the above in your completions list. Sounds like you're saying we should add a |
echo " Mac OS: brew install unzip" | ||
return 1 | ||
fi | ||
} | ||
|
||
_dvm_check_fzf() { | ||
if [[ ! -x "$(which fzf)" ]]; then | ||
echo "ERROR: dvm requires the 'fzf' tool, which was not found." | ||
echo "To install fzf:" | ||
echo " Debian/Ubuntu: sudo apt install fzf" | ||
echo " RedHat/Fedora: sudo dnf install fzf" | ||
echo " OpenSUSE: sudo zypper install fzf" | ||
echo " Arch/Manjaro: sudo pacman -S fzf" | ||
echo " Mac OS: brew install fzf" | ||
echo "" | ||
echo "See: https://github.com/junegunn/fzf#installation" |
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 think it'd be nice to use fzf
if it's installed, but not require it. I actually hadn't heard of it till today, but it looks pretty nice!
That raises the question of how we advertise to the user that dvm makes use of fzf
if available. Perhaps we could break out a dvm doctor
command and have it list all dependencies (required and optional) with their install status. We'd show a scary red X if a required dependency is missing (currently just unzip
), a yellow warning sign or something if fzf
is missing, otherwise a green check.
That would entail breaking out _dvm_check_required_deps
and _dvm_check_optional_deps
functions and then pull the two together in _dvm_doctor
. We'd then run _dvm_check_required_deps
on line 497 below instead of _dvm_doctor
.
What do you think?
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 only call _dvm_check_fzf
if we are about to use fzf
.
So if you do: dart use <version>
you'll never notice... If you do dart use
it'll error saying that you need fzf
.
I did consider improving the error message to say that dart use <version>
works without fzf
, but looking at how widely fzf
is available I think most users will just install it.
In either case, we could explicitly support --help
for all commands, but I think that's a separate issue :D
Yeah, or something like that.. I frequently find that what I want to do is:
There is many ways we could do this. Maybe Just because, |
@cbracken I assume we both forgot all about this 🤣 Should we merge... all the stuff about adding additional commands and shortcuts seems like follow up PRs, if we want to do it. With |
Just adding a note to apologise for the delay! I haven't forgotten about this, but was out for three weeks starting almost exactly when you last replied and finally got caught up on all the other stuff! Will take a look! |
@cbracken friendly ping :D |
This adds:
_
so they don't show up indv<tab><tab>
dvm use <tab><tab>
will show list of installed versions and--default
dvm use --default <tab><tab>
will show list of installed versionsdvm install <tab><tab>
will show list of stable versions (this seems like the thing most used will need).fzf
dvm install
will openfzf
with stable versionsdvm use
will optionfzf
with installed versionsdvm use --default
will optionfzf
with installed versionsTo make bash completion and fuzzy matching fast, the list of versions for stable, beta and dev will be saved to:
$DVM_ROOT/cache/versions-stable
$DVM_ROOT/cache/versions-beta
$DVM_ROOT/cache/versions-dev
If older than 5 minutes the list of versions will be refreshed. According to
man date
this should work on Mac OS too, but I only tested it on Linux.warning: it's entirely possible that I made a typo in some of this code, I renamed quite a few things -- and I have yet to figure out what
dvm alias
is good for 🙈