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

Vagrant completion updates #1748

Closed

Conversation

@zzamboni
Copy link
Contributor

@zzamboni zzamboni commented Oct 10, 2014

Improved vagrant completions for some commands to avoid spurious completions, and added completions for some missing Vagrant commands.

zzamboni added 3 commits Oct 10, 2014
Fix the comparisons in argument numbers to avoid fish providing commands
or subcommands as completions after they have been provided already.
Recent versions of vagrant produce additional output in `vagrant box
list`, which was introducing spurious text in the completions.
Add completions for recently-added commands connect, share,
global-status, login, rdp. Also improved the command comparisons to
improve completion of subcommands and their options.
return 0
end
end
return 1

This comment has been minimized.

@lilyball

lilyball Oct 10, 2014
Member

This could be simplified to

function __fish_vagrant_using_command
    set -l cmd (commandline -opc)
    set -q cmd[2]; and test "$argv[1]" = $cmd[2]
end

Note: set -l for cmd so we don't overwrite any global/universal variables with that name.

set -q cmd[2] is a quick way of testing to ensure $cmd[2] exists, and then we quote "$argv[1]" because we didn't test to make sure it exists and want to make sure we pass something to test; since we already verified $cmd[2] exists we don't need to quote it.

This comment has been minimized.

@zzamboni

zzamboni Oct 10, 2014
Author Contributor

Thank you, good tips - will fix.


function __fish_vagrant_using_command_and_no_subcommand
set cmd (commandline -opc)
if [ (count $cmd) -eq 2 ]
if [ $argv[1] = $cmd[2] ]
return 0
end

This comment has been minimized.

@lilyball

lilyball Oct 10, 2014
Member

In this function, set -q cmd[2] won't work unless you also test not set -q cmd[3] since you're trying to ensure there's only two words. So we can stick with count. But it can still be simplified (and the set cmd be fixed):

function __fish_vagrant_using_command_and_no_subcommand
    set -l cmd (commandline -opc)
    test (count $cmd) -eq 2 -a "$argv[1]" = "$cmd[2]"
end

"$cmd[2]" is quoted in this case because we're passing the argument to test regardless of whether the first test passes. We could use test (count $cmd) -eq 2; and test "$argv[1]" = $cmd[2] but there seems to be no point.

@@ -23,7 +33,7 @@ function __fish_vagrant_using_subcommand
set cmd_main $argv[1]
set cmd_sub $argv[2]

This comment has been minimized.

@lilyball

lilyball Oct 10, 2014
Member

These sets should use -l, for the same reason as above: so an inherited global/universal variable of that name doesn't get overwritten.

@@ -32,7 +42,7 @@ function __fish_vagrant_using_subcommand
end

This comment has been minimized.

@lilyball

lilyball Oct 10, 2014
Member

Actually the whole function can be simplified again:

function __fish_vagrant_using_subcommand --arguments cmd_main cmd_sub
    set -l cmd (commandline -opc)
    set -q cmd[3]; and test "$cmd_main" = $cmd[2] -a "$cmd_sub" = $cmd[3]
end

Looks like the if [ $cmd_main ... command was actually wrong to begin with, since it was using an ; and in the condition without a begin/end block. Which is to say, it was never actually testing that the subcommand was correct.

This comment has been minimized.

@zzamboni

zzamboni Oct 10, 2014
Author Contributor

Ooooh, this is the reason why the --provider flag was being offered as a completion to all the box subcommands! I wondered about that but never caught the mistake. Thanks!

This comment has been minimized.

@zzamboni

zzamboni Oct 10, 2014
Author Contributor

And I just noticed the --arguments - very nice! Much cleaner than the usual shell way of fetching arguments.

This comment has been minimized.

@lilyball

lilyball Oct 10, 2014
Member

Eh, when it works. It's basically equivalent to just

set -l cmd_main $argv[1]
set -l cmd_sub $argv[2]

except without array out of bound index errors.

But if you need to conditionally parse out flags or suchlike, it can end up being more annoying to work with than just $argv.

@@ -32,7 +42,7 @@ function __fish_vagrant_using_subcommand
end

function __fish_vagrant_boxes --description 'Lists all available Vagrant boxes'
command vagrant box list
command vagrant box list | awk '{print $1}'

This comment has been minimized.

@lilyball

lilyball Oct 10, 2014
Member

awk seems a tad overkill for this.

set -l IFS \n\ \t
command vagrant box list | while read -l name _
    echo $name
end

Granted, awk is shorter, but it involves invoking an extra process, when we can easily grab the first word of each line in fishscript.

zzamboni added 2 commits Oct 10, 2014
Simplifications and fixes as suggested by @kballard
Use `read` instead of `awk` to parse the box list.
@lilyball lilyball closed this in d982f2a Oct 10, 2014
@lilyball
Copy link
Member

@lilyball lilyball commented Oct 10, 2014

Squashed and pushed as d982f2a. Thanks!

@lilyball lilyball added this to the next-minor milestone Oct 10, 2014
@zzamboni
Copy link
Contributor Author

@zzamboni zzamboni commented Oct 10, 2014

Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants