-
Notifications
You must be signed in to change notification settings - Fork 19
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
ref(*): pass io.ReadCloser instead of string body #11
Conversation
defer func() { | ||
io.Copy(ioutil.Discard, res.Body) | ||
res.Body.Close() | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you want to schedule this after the ioutil.ReadAll
returns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I misread and assumed the defer
was actually a go
. ignore me
7dca4be
to
61cbe35
Compare
return "", ErrNoLogs | ||
} | ||
|
||
// We need to trim a few characters off the front and end of the string | ||
return body[2 : len(body)-1], nil | ||
return string(body[2 : len(body)-1]), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do a bounds check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now doing a bounds check in the previous if statement.
code LGTM after #11 (comment) is addressed |
61cbe35
to
bcca709
Compare
Requested by @arschles
Note that this PR removes the current system of error messages, because it is incompatible with the
io.ReadCloser
refactor.My work over the next few days will be to implement a new error system based on error constants that can be checked by sdk users and that will be compatible with
io.Readers
. If we don't want to temporarily remove verbose error messages, I can get this reviewed, then submit a new PR with both this refactor and the error messages refactor.Also note that this does not affect
LimitedRequest()
. This is because it needs to parse the json body in order to work, meaning that the response already has to be converted to a string. I'm open for ways that this could be better implemented.I'm returning the entire
*http.Response
object as opposed to just the body, because some of the information, such as response code, can be used to return specific error messages for an action with my error messages rewrite.