-
Notifications
You must be signed in to change notification settings - Fork 1
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
Introduce api.Error type (and improve errors) #64
base: master
Are you sure you want to change the base?
Conversation
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 not Michael Grace, but I thought I'd have a quick look while I'm browsing through URY stuff :) it looks mostly good to me, I just had a few questions about things.
res, err := client.Do(req) | ||
if err != nil { | ||
return &Response{err: err} | ||
return &Response{err: fmt.Errorf("failed to perform HTTP request: %w", err)} | ||
} | ||
defer 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.
Are there any errors that need to be caught on closing a request body?
@@ -178,38 +178,38 @@ func (s *authedRequester) Do(r *Request) *Response { | |||
theurl.RawQuery = encodedParams | |||
} | |||
req, err := http.NewRequest(reqMethod, theurl.String(), bytes.NewReader(r.Body.Bytes())) | |||
if err != 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.
I presume the old behaviour of setting the Content-Type
header before bailing for the error was a bug?
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 presume it would have been, I would imagine that NewRequest
would return (nil, error)
and thus we'd try to access Header
on a nil
req
.
} | ||
|
||
// Even if the result is non-200 we still want to try to unmarshal the response, to have a more descriptive error | ||
var response struct { |
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.
Are there any situations where the response coming back from MyRadio states that it is not OK in the payload but OK in the HTTP status, or vice versa? Am just wondering about the fact that there is now only one check at the response body level and nothing on the HTTP code directly.
(Presumably those would be bugs in MyRadio, mind. It seems kind of weird to have that redundancy, but it's been so long since I looked at MyRadio that I don't even know anymore.)
|
||
// Error is the error type returned when API requests result in an error. | ||
type Error struct { | ||
Endpoint string |
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.
Could the fields be commented, eg. making clear that Code
is the HTTP status code coming back from the MyRadio response but Payload
is (I guess) the raw description string coming out of MyRadio?
I ran into the problem of missing error granularity again while trying to work on 2016-site's podcast handling, so it'd definitely be nice to have. |
This will let downstream consumers handle the different MyRadio response codes differently (for example, 2016-site rendering a 404 when MyRadio sends a 404, rather than a 404 for everything). Intended usage is