Closed russcam closed 4 years ago
I think it would be best to mirror reqwest and add an error_for_status
method to get the best of both worlds.
This sounds like a good idea. I’ll have a chance to look at this next week when I’m back
I've had a chance to dig into this a little more. There's a few things that are worth discussing, I think:
For some APIs, one might consider status codes that imply an error to be a semantically valid response. Take for example
In the .NET client, there is an IsValid
property exposed on the response that indicates when a response is one that is valid for the API in question. Should this client expose something similar?
Error
? top level Error
is now a struct (#41), so maybe this makes sense?error_for_status_code()
function, beyond the consumer checking status_code()
themselves. (This is as much a rhetorical question as it is an open one!) Lots to think about...
For some APIs, one might consider status codes that imply an error to be a semantically valid response.
I see your point but part of me wants to just say “document it and let the user handle it”.
the index exists API; a 404 means the index does not exist. Should a 404 be considered an error in this case?
I could conceive of me wanting that to be an error that I can easily bubble up to the user, but other times I would want to handle it myself and error_for_status_code
would give me that flexibility to decide when.
the bulk API returns a 200 response, even when there can be errors with some of the operations.
This is actually a rough spot for me when using the elastic
crate. While it might not be the most secure thing to do, I am currently (at least not in production yet) mapping over all of the bulk “errors” and putting them all in one Err
that I send back via the graphql API.
All that being said, I definitely see your point... No matter what we decide we will need to be very clear in the naming, docs etc. what is going on.
Should this be exposed on Error?
100% yes. I am still thinking about how to best handle that (in terms of mapping the responses to rust types etc), but improving error messages is among my highest priorities. (It might even warrant its own issue)
Are there other properties that it makes sense to expose?
I see what you are getting at. I think so? I am not sure what the added functionality should be at the moment though.
I will do more thinking on all of this...
All HTTP status code responses currently return
Ok(Response)
inResult<Response, Error>
.Should status codes 400-599 return
Err(Error)
?