-
Notifications
You must be signed in to change notification settings - Fork 282
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
RFC: less opaque error type #31
Comments
Hi @mspiegel, thank you for opening an issue to discuss this first, it's very helpful. Yes, a less opaque error type makes sense as a next step to consider. I already alluded to it in #29 (comment):
I'm happy to proceed discussing this, but I would like to gather some data on the use cases first. These might seem like obvious questions, but I want to ask them anyway. Who would use this? How would they benefit? Are these situations hypothetical, or have you actually encountered them? I would like to implement this only if it solves real problems. Understanding what those are will help us evaluate potential solutions and make sure we pick a good one. So, can you please elaborate on why you want this, and what can't be done right now (or can only be done inconveniently). |
Sure. I'm building a GitHub integration. So it will listen for events from
GitHub, it may perform actions based on those events, and then it returns
an http response.
Incidentally I don't know what is the best practice on sync vs async GitHub
integrations. Sync makes it easier to debug because the webhook responses
show up in the GitHub UI. I've informally heard that GitHub prefers async
to avoid timeouts and also their servers don't want to wait for my service
to complete.
Let's assume I continue with sync responses. In cases of an error I need to
return a message body and an http status code. When I was using the GitHub
V3 API then their status code became my status code. This works out
remarkably well. It has the additional benefit that I can treat 400-level
responses as warnings that don't require immediate attention. And 500-level
responses as errors that will trigger a notification to me.
I don't really have that nice demarcation of error types in the GitHub v4
API. With the current error handling in this library, I get even less
information back than I could be getting. As a first implementation, I
think that I could treat any GraphQL error responses (the errors array
type) as warnings. And any other kind of error, like JSON serde errors, as
errors. I might be able to take advantage of the http response code
although it's not defined when GitHub returns a non-200 response.
|
* use hyphen tag to skip printing fields to the query string output
I want to submit a pull request that provides a less opaque error type. This is similar to #29 but has a broader purpose. I want to get some feedback before I submit a pull request.
Can I export the errors type? That will allow me to do something with the Message and Locations.
How do I provide more information about where Client.do() is generating the error? I see 6 locations where an error is returned. Which is a better approach:
a) Should I declare a different error type for each of these conditions? Is that the idiomatic Go way? That appears to be what the golang standard library does.
b) Should I use an enum and wrap the error like this https://play.golang.org/p/xPca1LmOBbp? One advantage of this approach is that (unlike option a) we aren't declaring a new error type that is just some wrapper for another error type (like a json error type). The enum describes where the error occurs not the shape of the error.
c) Something else?
The text was updated successfully, but these errors were encountered: