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.

Sign up for GitHub

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

Jump to bottom

error handling refactor - wip #84

Merged
merged 4 commits into from
May 7, 2013
Merged

error handling refactor - wip #84

merged 4 commits into from
May 7, 2013

Conversation

steved
Copy link
Contributor

@steved steved commented Mar 26, 2013

@3en @benilovj

Since I've heard mostly complaints about the current error handling, I've begun to try to refactor it for a 1.0-type release. I'll be working on this branch for the next couple days to try and make it more consistent / cleaner, but may merge it within the week. I'm hoping this will solve most of your error-handling issues, but I'd love your feedback.

Notably:
#71, #76

Becomes:

begin
  subject.each_page! { .... } # Note the "!"
rescue ZendeskAPI::Error::NetworkError
  retry
rescue ZendeskAPI::Error::ClientError
end

Which will only retry on Network errors (400..600, not including 422) and always start from the correct page.
#72

@steved
error handling refactor
7f4c236
@steved
support
448a205 
```
begin
  subject.each_page!(&b)
rescue ZendeskAPI::Error::NetworkError
  retry
rescue ZendeskAPI::Error::ClientError
end
```
@benilovj
Copy link
Contributor

that looks good.. would definitely be a big improvement. Thanks!

@3en
Copy link

3en commented Mar 27, 2013

Ditto, Thanks @steved555 on face value this looks good.

I will do some more testing.

craiglittle
craiglittle reviewed Apr 1, 2013
View reviewed changes
spec/core/middleware/response/raise_error_spec.rb
let(:status) { 404 }

it "should raise RecordNotFound when status is 404" do
expect { client.connection.get "/non_existant" }.to raise_error(ZendeskAPI::Error::RecordNotFound)
Copy link

Choose a reason for hiding this comment

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

existent :)

steved added a commit that referenced this pull request May 7, 2013
@steved
Merge pull request #84 from zendesk/errors
cadf8b0 
error handling refactor - wip
@steved steved merged commit cadf8b0 into master May 7, 2013
@steved steved deleted the errors branch May 7, 2013 21:32
@benilovj benilovj mentioned this pull request May 29, 2013
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@steved @benilovj @3en @craiglittle