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

fix: ensure that testproject is removed even after a failure #948

Merged
merged 4 commits into from
Sep 17, 2019

Conversation

umarcor
Copy link
Contributor

@umarcor umarcor commented Sep 5, 2019

In 984374f a defer func() was added after a possible t.Fatal(err). This PR moves it before, to ensure that testproject files are removed after a failure.

@jharshman jharshman added the kind/bug A bug in cobra; unintended behavior label Sep 7, 2019
@jharshman
Copy link
Collaborator

jharshman commented Sep 7, 2019

Looks like a good bug fix @umarcor. Got my 👍but will give others a chance to review the change as well. I know it's not part of your change-set but If you don't mind, I think that defer statement can be simplified.

@umarcor
Copy link
Contributor Author

umarcor commented Sep 7, 2019

Thanks @jharshman!

I know it's not part of your change-set but If you don't mind, I think that defer statement can be simplified.

I wouldn't mind to do so, but I'm sleepy now and I don't understand what you mean. How do you suggest to simplify it? Note that I would apply the simplification to add_test.go too (which is the other file that was modified in 984374f).

@jharshman
Copy link
Collaborator

How do you suggest to simplify it?

According to the package docs, os.RemoveAll(path string) doesn't fail when the path doesn't exist. Therefore you should be able to simplify to defer os.RemoveAll(project.AbsolutePath).

ref: https://golang.org/pkg/os/#RemoveAll

@umarcor
Copy link
Contributor Author

umarcor commented Sep 8, 2019

Thanks for the hint @jharshman! I updated the PR accordingly.

cobra/cmd/init_test.go Outdated Show resolved Hide resolved
@umarcor umarcor force-pushed the fix-testproject branch 2 times, most recently from 141230d to 941bb48 Compare September 17, 2019 12:59
@jharshman jharshman merged commit 19cf35e into spf13:master Sep 17, 2019
@umarcor umarcor deleted the fix-testproject branch September 17, 2019 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in cobra; unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants