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

Add basic bash completion script and framework for others #44

Merged
merged 1 commit into from
Jul 31, 2017

Conversation

scotje
Copy link
Contributor

@scotje scotje commented Jul 26, 2017

No description provided.

hostname_subcommands="delete modify query revert snapshot"

if [[ $template_subcommands =~ (^| )$prev($| ) ]] ; then
if [ -z "$_vmfloaty_avail_templates" ] ; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using the extended test ([[) instead of classic test ([); should be essentially a drop-in here.


if [[ $template_subcommands =~ (^| )$prev($| ) ]] ; then
if [ -z "$_vmfloaty_avail_templates" ] ; then
_vmfloaty_avail_templates=`floaty list`
Copy link
Contributor

@mckern mckern Jul 26, 2017

Choose a reason for hiding this comment

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

Consider $() instead of ``; you used it elsewhere already so it's consistent.

COMPREPLY=( $(compgen -W "${subcommands}" -- ${cur}) )
fi
}
complete -F _vmfloaty floaty
Copy link
Contributor

Choose a reason for hiding this comment

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

Run this through shellcheck?

@mckern
Copy link
Contributor

mckern commented Jul 26, 2017

This is a good feature. I only have some goofball nitpicks and I don't think they're worth blocking on.

@scotje
Copy link
Contributor Author

scotje commented Jul 26, 2017

I can fix those issues up real quick. Also realized I should reject completion if cur starts with a - since this doesn't support flags/options yet.

@scotje
Copy link
Contributor Author

scotje commented Jul 26, 2017

Fixed up noted issues and a couple other little things.


```bash
ln -s $(floaty completion --shell bash) /usr/local/etc/bash_completion.d/floaty
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm. Shortcoming: if you've installed floaty as a gem or via the Homebrew tap I smashed together last year, this won't work.

mckern@flexor vmfloaty (git:add_bash_completion) $ (set -x; floaty completion --shell bash)
+ floaty completion --shell bash
mckern@flexor vmfloaty (git:add_bash_completion) $ 

extras/completions/floaty.bash isn't actually installed anywhere right now by the gem, so it can't be referenced. Thoughts?

@scotje
Copy link
Contributor Author

scotje commented Jul 26, 2017

Oh, I think I just need to add the extras folder to the list of files in the gemspec.

@scotje
Copy link
Contributor Author

scotje commented Jul 26, 2017

OK, extras/completions should now actually be included in the .gem :)

@mckern
Copy link
Contributor

mckern commented Jul 26, 2017

Can confirm:

mckern@flexor vmfloaty (git:add_bash_completion) $ (set -x; floaty completion)
+ floaty completion
/usr/local/var/rbenv/versions/2.3.4/lib/ruby/gems/2.3.0/gems/vmfloaty-0.7.8/extras/completions/floaty.bash

@mckern
Copy link
Contributor

mckern commented Jul 26, 2017

Ping @briancain; when this is in and a release is cut, I'll update my homebrew tap.

@scotje
Copy link
Contributor Author

scotje commented Jul 26, 2017

❤️ Thanks for all the feedback @mckern !

@briancain
Copy link
Contributor

@scotje woot thanks for the contribution! I'll check this out later this morning and try it :)

Copy link
Contributor

@briancain briancain left a comment

Choose a reason for hiding this comment

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

Overall 👍 on this, I'm happy to see this tool still live on 💃 Just got a simple change for the help strings and then a question about maybe printing an error to STDERR if an autocomplete file is missing but otherwise looks awesome!

lib/vmfloaty.rb Outdated
command :completion do |c|
c.syntax = 'floaty completion [options]'
c.summary = 'Outputs path to completion script'
c.description = "Outputs path to a completion script for the specified shell, or bash if not specified.\nWill exit non-zero if no completion script is available for the requested shell."
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should include the help text from the readme here just so people understand how to use this command (i.e. sourcing the file this command outputs) just so when people do floaty completion --help they have some idea what to do with the command.

lib/vmfloaty.rb Outdated
puts completion_file
exit 0
else
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense here to print a warning to STDERR in the case where the file requested is missing?

@briancain
Copy link
Contributor

Thanks for the contribution @scotje and @mckern for feedback!

@scotje
Copy link
Contributor Author

scotje commented Jul 27, 2017

Updated with better --help output and an error message.

@scotje
Copy link
Contributor Author

scotje commented Jul 29, 2017

ping @briancain

Thanks for taking a look and have a nice weekend!

Copy link
Contributor

@briancain briancain left a comment

Choose a reason for hiding this comment

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

👍

@briancain briancain merged commit 2cd2bd1 into puppetlabs:master Jul 31, 2017
@briancain
Copy link
Contributor

Thanks @scotje ! Your changes are now in 0.7.9 which I just released (cc @mckern). It should be there if you do a gem update, or however you have it installed!

@scotje scotje deleted the add_bash_completion branch July 31, 2017 17:42
@scotje
Copy link
Contributor Author

scotje commented Jul 31, 2017

Thanks Brian! ❤️

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.

3 participants