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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

New `vagrant pristine` command #5613

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@fgrehm
Collaborator

fgrehm commented Apr 21, 2015

Closes GH-5378

Ideally we should just reuse the other command plugins instead of copying and pasting but its been a while since I been around the codebase and I can't remember if it would be possible to do that 馃槉

/cc @sethvargo

DESC
command("pristine") do
require File.expand_path("../command", __FILE__)

This comment has been minimized.

@sethvargo

sethvargo Jun 1, 2015

Contributor

Let's use require_relative instead please

@sethvargo

sethvargo Jun 1, 2015

Contributor

Let's use require_relative instead please

end
# Success, exit status 0
0

This comment has been minimized.

@sethvargo

sethvargo Jun 1, 2015

Contributor

Explicit return here would be nice 馃槃

@sethvargo

sethvargo Jun 1, 2015

Contributor

Explicit return here would be nice 馃槃

@sethvargo

This comment has been minimized.

Show comment
Hide comment
@sethvargo

sethvargo Jun 1, 2015

Contributor

Hey @fgrehm

Thank you for the Pull Request. At a high level, this looks really great. Do you think you could add some tests for this? If you look at the pushes tests, I think you can find a good pattern to use. I'm just really hesitant to merge such a large change without test coverage. Even if it works now, we need to prevent regressions in the future 馃槃

Contributor

sethvargo commented Jun 1, 2015

Hey @fgrehm

Thank you for the Pull Request. At a high level, this looks really great. Do you think you could add some tests for this? If you look at the pushes tests, I think you can find a good pattern to use. I'm just really hesitant to merge such a large change without test coverage. Even if it works now, we need to prevent regressions in the future 馃槃

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Jun 1, 2015

Collaborator

Sure! I'll try to write those tests over the week.

Collaborator

fgrehm commented Jun 1, 2015

Sure! I'll try to write those tests over the week.

@mitchellh

This comment has been minimized.

Show comment
Hide comment
@mitchellh

mitchellh Jul 9, 2015

Member

Thanks for this. I'm not a big fan of the name, I don't know what to expect with pristine, for example I still have no idea what gem pristine does. I know what "pristine" means but for some reason it has never resonated with me as a command name. I still think this makes sense as a flag to reload.

Member

mitchellh commented Jul 9, 2015

Thanks for this. I'm not a big fan of the name, I don't know what to expect with pristine, for example I still have no idea what gem pristine does. I know what "pristine" means but for some reason it has never resonated with me as a command name. I still think this makes sense as a flag to reload.

@mitchellh

This comment has been minimized.

Show comment
Hide comment
@mitchellh

mitchellh Jul 9, 2015

Member

.@sethvargo is trying to convince me this should be a command and not a flag. We'll delay this for 1.8 in some form.

Member

mitchellh commented Jul 9, 2015

.@sethvargo is trying to convince me this should be a command and not a flag. We'll delay this for 1.8 in some form.

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Jul 10, 2015

Collaborator

No worries! I'm not sure when I'll have a chance to write tests for this so feel free to close the PR if you guys want to hold back on it! 馃嵒

Collaborator

fgrehm commented Jul 10, 2015

No worries! I'm not sure when I'll have a chance to write tests for this so feel free to close the PR if you guys want to hold back on it! 馃嵒

@garrettr garrettr referenced this pull request Jul 27, 2015

Closed

Audit vagrant plugins #1045

@sethvargo sethvargo added this to the 1.8 milestone Nov 20, 2015

@sethvargo

This comment has been minimized.

Show comment
Hide comment
@sethvargo

sethvargo May 27, 2016

Contributor

Hey @fgrehm

Thanks for the PR. Coming around back to this (almost a year later), I think I've had a change of heart. After reading through all the comments in the original issue, I think everyone's opinions on "what this should be named" and "what it should do" are too far apart. As such, I'm going to recommend folks use a shell alias to involve whatever their preferred behavior is, instead of building this into core. I totally appreciate your efforts on this PR, but I'm actively working to simplify Vagrant's CLI API, and I don't think it's wise to add new commands at this time, especially since this "works" with some &&. Sorry!

Contributor

sethvargo commented May 27, 2016

Hey @fgrehm

Thanks for the PR. Coming around back to this (almost a year later), I think I've had a change of heart. After reading through all the comments in the original issue, I think everyone's opinions on "what this should be named" and "what it should do" are too far apart. As such, I'm going to recommend folks use a shell alias to involve whatever their preferred behavior is, instead of building this into core. I totally appreciate your efforts on this PR, but I'm actively working to simplify Vagrant's CLI API, and I don't think it's wise to add new commands at this time, especially since this "works" with some &&. Sorry!

@sethvargo sethvargo closed this May 27, 2016

@fgrehm fgrehm deleted the f-pristine-cmd branch May 30, 2016

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm May 30, 2016

Collaborator

No need to be sorry! 馃嵒

Collaborator

fgrehm commented May 30, 2016

No need to be sorry! 馃嵒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment