Make TravisCI build green again #62
Conversation
gometalinter/golint raised some warnings about the naming of struct fields: gometalinter --config gometalinter.json ./... changes.go:261:2:warning: struct field RobotId should be RobotID (golint) changes.go:263:2:warning: struct field RobotRunId should be RobotRunID (golint) changes.go:265:2:warning: struct field Url should be URL (golint) changes.go:280:2:warning: struct field RobotId should be RobotID (golint) changes.go:282:2:warning: struct field RobotRunId should be RobotRunID (golint) changes.go:284:2:warning: struct field Url should be URL (golint) changes.go:297:2:warning: struct field FixId should be FixID (golint)
Codecov Report
@@ Coverage Diff @@
## master #62 +/- ##
=========================================
Coverage ? 22.48%
=========================================
Files ? 21
Lines ? 1801
Branches ? 0
=========================================
Hits ? 405
Misses ? 1341
Partials ? 55
Continue to review full report at Codecov.
|
changes.go
Outdated
| @@ -780,8 +780,15 @@ func (s *ChangesService) change(tail string, changeID string, input interface{}) | |||
|
|
|||
| v := new(ChangeInfo) | |||
| resp, err := s.client.Do(req, v) | |||
| if err != nil { | |||
| return v, resp, err | |||
There was a problem hiding this comment.
Other similar lines (above) do return nil, resp, err. Better to be consistent.
changes.go
Outdated
| body, err = ioutil.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return v, resp, err | ||
| } | ||
| err = errors.New(string(body[:])) | ||
| } | ||
| return v, resp, err |
There was a problem hiding this comment.
I would suggest rewriting as follows to make the code more readable:
...
if resp.StatusCode == http.StatusConflict {
body, err := ioutil.ReadAll(resp.Body)
if err != nil {
return v, resp, err
}
return v, resp, errors.New(string(body[:]))
}
return v, resp, nilAlso, I'm suspect about this whole thing working. Doesn't s.client.Do(req, v) already consume the resp.Body before returning? There are no tests for it, and no rationale for why it was added in #15. I'd remove it and simplify this endpoint to be like all others. But it's out of scope of this PR.
There was a problem hiding this comment.
I adjusted the code according to your suggestion.
Regarding your comment: Indeed. That could be a possible mistake. And I am with you, that this has to be investigated in a separate ticket/pr. Let me check this, and I will ping you for a review again.
There was a problem hiding this comment.
Hey @dmitshur, i am a little bit packed with private things the next days/weeks. I will come back to this later. If you have some spare time around, feel free to make the change. Otherwise i can do it later. Sorry, but thanks for understanding.
There was a problem hiding this comment.
Not at a problem at all, this isn't blocking me in any way.
If you don't want this opportunity to improve code to be forgotten, I'd suggest opening an issue to track it.
There was a problem hiding this comment.
Just to not forget it: #64 (was short on time, but i think it is enough for us now).
| if err != nil { | ||
| return v, resp, err | ||
| } | ||
| return v, resp, errors.New(string(body[:])) |
There was a problem hiding this comment.
A trivial thing I just spotted: body[:] could be simplified to body. It's already a slice, no need to slice it again.
This PR makes the TravisCI build green again. In detail
gastogosecOne important note: The renaming of struct fields is a breaking change. It was introduced in #60. @achew22 would be nice if you can check this as well, because it might break your code.