Skip to content
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

Add more structure to errors #11

Open
mrjones opened this issue Sep 17, 2013 · 3 comments
Open

Add more structure to errors #11

mrjones opened this issue Sep 17, 2013 · 3 comments

Comments

@mrjones
Copy link
Owner

mrjones commented Sep 17, 2013

Carrying over @deet's comment from #9 to a new bug:

I've run into some other issues with using this function and those that return its
result, and I'm not sure how they should be addressed. (Let me know if you want
this as a separate issue.) Specifically, clients can't determine whether the call
failed due to network error or because of something application-level (like non-2XX
response). So far it seems necessary to do a substring match for "HTTP response
is not 200/OK as expected" to determine that the call failed due to a server
response. Any suggestions?

Although I'm not entirely sure the best way to handle this canonically in Go, here's what I'm thinking:

http://blog.golang.org/error-handling-and-go has a section called "The error type". That section stars by discussing creating errors with errors.New and fmt.Errorf, which is basically what this library is doing now.

Starting with the "In many cases fmt.Errorf is good enough, but..." section, it goes on to describe defining an error struct, and then allowing callers to examine the details of that error using type assertion.

So here are two ideas along those lines:

  1. Define a single new error type "OauthError" that extends the error type, and we can insert any additional information in there. I'm not 100% sure what would go in the struct, but you could imagine something like an httpStatusCode field, that was empty if no HTTP request was made.
  2. Define multiple new error types OauthHttpError, OauthNetworkError, GenericOauthError, whatever. Then the user can do type assertion and figure out which kind of error they got back.

My first reaction is that the first option involves less type assertion, so seems a little cleaner, but both seem like they'd be totally fine.

If you're want to, feel free to send a patch. Otherwise I'll try to write some code for this maybe in the next week or two.

@deet
Copy link

deet commented Sep 18, 2013

@mrjones I'll help with this. Option 1 sounds good and could be extended to option 2 in the future if needed.

I'll try to prepare a patch to get us going, or at least better evaluate the options, but it might be a few days.

@deet
Copy link

deet commented Oct 1, 2013

Progress so far: https://github.com/deet/oauth/compare

Still need to populate the extra error info in the rest of the API where appropriate.

With these changes, users would not need to change their existing code. Optionally, they can type assert the errors to check the extra info, like:

if err != nil && err.(*oauth.OAuthError).HttpStatusCode() == 0 {
...
}

@mrjones
Copy link
Owner Author

mrjones commented Oct 7, 2013

Hey! Sorry for not commenting sooner ... it slipped through the cracks. However, this looks great. Thanks for taking the time to work on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants