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

Support new Vagrant commands #2787

Merged
merged 5 commits into from
May 16, 2014
Merged

Conversation

irnnr
Copy link
Contributor

@irnnr irnnr commented May 6, 2014

Vagrant 1.5 and 1.6 added a couple new commands including version, share and connect, login, and global-status. This PR adds these commands to zsh completion.

irnnr added 4 commits May 6, 2014 10:45
Vagrant 1.6 introduces a couple new commands, including the `version` command.
The `version` command shows the currently installed version information and
also checks for new updates available.
Vagrant 1.5 introduced Vagrant Share to allow remote access to a Vagrant
environment. This adds support for the `share` and `connect` commands.
Vagrant 1.5 added Vagrant Cloud to share boxes. Some boxes
may be protected, the `login` command allows to access those
protected boxes from Vagrant Cloud.
Vagrant 1.6 introduced the `global-status` command which allows
to get a quick overview of all active Vagrant environments for the
currently logged in user.
@ncanceill
Copy link
Contributor

Hi, and thanks for contributing.

The vagrant plugin is in need of maintenance. Before submitting new features, maybe you could try to fix the currently existing issues — eg #2797.

Also, if you want this PR to have a chance of being pulled, you should try to find vagrant plugin users to test your changes.

@irnnr
Copy link
Contributor Author

irnnr commented May 15, 2014

Hi Nicolas,

I think both are important, fixing existing issues, but also this one. Making an unrelated issue's fix a requirement doesn't sound very convincing. I can live with and get other user's feedback for this issue of course. If you look at the changes you see that the PR really just adds to the existing completions.

Concerning #2797, yes that'd probably be nice but also a lot more complicated compared to this PR. I have run into a similar issue where no completion is offered past the first parameter and would like that to be working, too. I actually might go and try to implement that. However, as I said I see no relation between the two issues.

@ncanceill
Copy link
Contributor

I understand that your changes are simple, but even though it makes them easy to review, it does not exonerate them from being reviewed: you may have inadvertently duplicated an option, or made a typo...

Making an unrelated issue's fix a requirement doesn't sound very convincing.

Yes, it was not very well thought on my part to use the word "before", sorry about that. What I meant is that, since you seem willing to contribute to that plugin, there is more work just waiting for you to pick it up. ;-)

'destroy:Destroys the vagrant environment'
'global-status:Reports the status of all active Vagrant environments on the system.'
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of consistency, you may want to remove the final dot on that line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@irnnr
Copy link
Contributor Author

irnnr commented May 15, 2014

@ncanceill yes, that sounds a lot better ;) Definitely willing to contribute more if I can. Also no doubt that changes need reviews! Unfortunately I could not find a clear contribution workflow description for ohmyzsh

@mcornella
Copy link
Member

Unfortunately I could not find a clear contribution workflow description for ohmyzsh

This is a Work In Progress: #2766 (more like Future Work In Progress), but still 😄

@irnnr
Copy link
Contributor Author

irnnr commented May 15, 2014

@mcornella thanks for referencing that issue!

@ghost
Copy link

ghost commented May 15, 2014

works for me.

@paulrohrbeck
Copy link

@irnnr's changes looks fine to me

@stucki
Copy link
Contributor

stucki commented May 16, 2014

Works! Thanks Ingo! 👍

@dfeyer
Copy link

dfeyer commented May 16, 2014

👍 Work for me

robbyrussell added a commit that referenced this pull request May 16, 2014
@robbyrussell robbyrussell merged commit 3913106 into ohmyzsh:master May 16, 2014
@irnnr irnnr deleted the plugin-vagrant branch May 16, 2014 16:39
@irnnr
Copy link
Contributor Author

irnnr commented May 16, 2014

thanks guys!

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.

7 participants