Handle rate limits #354
Handle rate limits #354
Conversation
Gemfile
Outdated
| gemspec | ||
| %w[rspec rspec-core rspec-expectations rspec-mocks rspec-support].each do |lib| | ||
| gem lib, :git => "git://github.com/rspec/#{lib}.git", :branch => 'master' | ||
| end |
There was a problem hiding this comment.
Are we using rspec anywhere? Thought the tests here are running via mocha.
There was a problem hiding this comment.
Good point, initially I thought test was failing due to missing rspec dependency but like you said, turns out it was mocha so can remove this
| require 'webmock' | ||
| include WebMock::API | ||
|
|
||
| WebMock.enable! |
There was a problem hiding this comment.
Can we shift these imports + configuration into the spec_helper file? webmock can come in handy for other tests in the future.
There was a problem hiding this comment.
Yeah, I was initially unsure whether to put it in spec_helper file in case it impacted other tests but will do it now. It should not impact other tests
| raise_errors_on_failure(response) | ||
| parsed_body | ||
| rescue Intercom::RateLimitExceeded => e | ||
| if @handle_rate_limit |
There was a problem hiding this comment.
We'll need a max retry mechanism here. Unlikely scenario but we can risk running into an infinite loop if we don't have one.
There was a problem hiding this comment.
makes sense, any suggestion on max retry? 10 or 20 or something like that?
There was a problem hiding this comment.
Yup. We wait for a fair bit here so I would go with something small like 3. This operation risks getting too long otherwise. Thoughts @mmartinic ?
There was a problem hiding this comment.
3 sounds good. we can make this configurable later
lib/intercom/request.rb
Outdated
| end | ||
|
|
||
| def execute(target_base_url=nil, username:, secret: nil) | ||
| retries ||= 3 |
There was a problem hiding this comment.
this is outside of try block. we can just do retries = 3
There was a problem hiding this comment.
Yeah, initially i had it in the try block but moved it outside and forgot to change it
README.md
Outdated
| #=> {:limit=>180, :remaining=>179, :reset_at=>2014-10-07 14:58:00 +0100} | ||
| ``` | ||
|
|
||
| You can handle the rate limits yourself but a simply option is to use the handle_rate_limit flag. |
|
@sureshs592 nice catch ! updated that now, do you think this is ok to go now? |
|
@sureshs592 set the rate limit flag in the init function now to make it clear what the default is |
Resolving conflicts for PR #264 and adding unit tests.
Also updating some other tests due to warnings