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

Don’t throw errors for “normal” errors #56

Merged
merged 1 commit into from
Jan 21, 2017

Conversation

cpjolicoeur
Copy link
Contributor

Return the more-standard elixir tuple of
{:error, message} and let the user decide what
they want to do with it.

We still raise hard errors for truly unrecoverable
things like missing keys and/or configuration
options.

[Fix #52]

Return the more-standard elixir tuple of
{:error, message} and let the user decide what
they want to do with it. 

We still raise hard errors for truly unrecoverable
things like missing keys and/or configuration
options.
@cpjolicoeur
Copy link
Contributor Author

@parroty This is an initial pass. Let me know what you think and what needs revisions.

@coveralls
Copy link

coveralls commented Sep 5, 2016

Coverage Status

Coverage increased (+0.2%) to 80.788% when pulling ff2d222 on cpjolicoeur:cpj/error_tuples into 2190725 on parroty:master.

Copy link
Contributor

@tomtaylor tomtaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, I think it's a good start and we should try and land something like this before extwitter hits 1.0.

code: code, message: message, reset_at: reset_at, reset_in: reset_in
_ ->
raise ExTwitter.Error, code: code, message: message
{:error, "RateLimitExceeded: #{message}, reset at: #{reset_at}, reset in: #{reset_in}"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this, and other similar places, we're losing a lot of exception information by throwing it all into a string. reset_at and reset_in are used by clients to back off connections until the rate limiter allows, so it's useful to have this information in the tuple returned. Perhaps we should be doing the following instead?

{:error, :rate_limit_exceeded, %{code: code, message: message, reset_in: reset_in, reset_at: reset_at}}

I'm not sure if throwing all this info into an error tuple is the most 'Elixary' way of doing this? Has anyone got better examples from other projects?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having the http status code would be helpful as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this PR was only to mimic the existing error messaging behavior while converting to stop throwing errors. Send back a tuple or map with extra metadata is greatly preferred.

@yammine
Copy link

yammine commented Sep 24, 2016

@tomtaylor I've recently tackled this change in ExTrello

https://github.com/ChrisYammine/ex_trello/blob/master/lib/ex_trello/api/base.ex

Maybe it can act as inspiration :)

@tomtaylor
Copy link
Contributor

@chrisyammine thanks - that looks useful!

@parroty
Copy link
Owner

parroty commented Dec 28, 2016

Very sorry being late to respond. I'll be looking into this item as for v0.8.0.

@parroty parroty merged commit ff2d222 into parroty:master Jan 21, 2017
@parroty parroty mentioned this pull request Jan 22, 2017
@parroty
Copy link
Owner

parroty commented Jan 22, 2017

Hi. I was working on this item for pushing 0.8.0, but couldn't well resolve the following concerns.

  • Treating errors as string which @tomtaylor mentioned in above.
  • Some endpoints (other than ExTwitter.Search) have similar problems.

I'm afraid but please let me revert tentatively, and rework on this item. I'm checking the example @chrisyammine provided, and thinking about similar approach (with this macro approach, I'm wondering if method! type of signature can be added too).

Sorry again for leaving it long time and making like this one.

@preslavrachev
Copy link

@parroty @cpjolicoeur Hey guys, has there been any movement on this issue? I suppose, by now, it will become a breaking change for many consumers, but I would really love to see trivial, recoverable errors being returned to the customer instead of potentially nuking an entire process.

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

Successfully merging this pull request may close these issues.

Return {:error, err} tuples instead of throwing errors
7 participants