status: handle errors when writing output #1420
Conversation
cmd/dep/status.go
Outdated
| } | ||
|
|
||
| out.BasicFooter() | ||
| err = out.BasicFooter() |
There was a problem hiding this comment.
Let's call this error variable something else and have explicit not nil check and return for it. Because reassigning this variable here makes the previous values (line 577 & 579) never being used. And the following return seems to return errCount, which is not related to this outputter error.
There was a problem hiding this comment.
got it, I'll have it return separately rather than continuing to the end if it errors there. (as it is doing with BasicHeader(), BasicLine(), etc)
darkowlzz
changed the title
CHANGELOG.md
Outdated
| @@ -20,6 +20,7 @@ IMPROVEMENTS: | |||
| * Make the gps package importable ([#1349](https://github.com/golang/dep/pull/1349)). | |||
| * Improve file copy performance by not forcing a file sync (PR #1408). | |||
| * Skip empty constraints during import ([#1414](https://github.com/golang/dep/pull/1349)) | |||
| * Handle errors when writing output ([#1420](https://github.com/golang/dep/pull/1420)) | |||
There was a problem hiding this comment.
Seems to be very general. Let's add "status output" to make it clear.
There was a problem hiding this comment.
ah, good point!
cmd/dep/status.go
Outdated
| err := out.BasicHeader() | ||
| if err != nil { | ||
| return false, 0, err | ||
| } |
There was a problem hiding this comment.
This can be shortened to:
if err := out.BasicHeader(); err != nil {
return false, 0, err
}Sorry that I didn't notice and suggest this in the first review.
There was a problem hiding this comment.
Oooh! That's nice and compact!
cmd/dep/status.go
Outdated
| err := out.BasicLine(bsMap[string(proj.Ident().ProjectRoot)]) | ||
| if err != nil { | ||
| return false, 0, err | ||
| } |
There was a problem hiding this comment.
Same as above, can be shortened.
cmd/dep/status.go
Outdated
| footerErr := out.BasicFooter() | ||
| if footerErr != nil { | ||
| return false, 0, footerErr | ||
| } |
There was a problem hiding this comment.
Same as above, can be shortened.
cmd/dep/status.go
Outdated
| err = out.MissingHeader() | ||
| if err != nil { | ||
| return false, 0, err | ||
| } |
There was a problem hiding this comment.
Same as above, can be shortened.
cmd/dep/status.go
Outdated
| } | ||
| err = out.MissingFooter() | ||
| if err != nil { | ||
| return false, 0, err |
There was a problem hiding this comment.
Same as above, can be shortened.
darkowlzz
changed the title
|
Thanks for fixing this. 👍 |
What does this do / why do we need it?
Addresses #1410 which is adding error handling to
status.goWhat should your reviewer look out for in this PR?
Is this done correctly? The errors get passed back in the outputters, and then I try to handle them in
runStatusAllwhen they're called.Do you need help or clarification on anything?
Suggestions on tests or any general feedback. This my first PR to this. (I'm also in the vendor channel on the golang slack.)
Which issue(s) does this PR fix?
fixes #1410