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

Accept Proc in the :on argument #55

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* Replace `minitest` gem with `rspec`
* Fancier README
* Remove unnecessary short circuit in `randomize` method
* Add ability for `:on` argument to accept a `Hash` where the keys are exception types and the values are either `Proc`s or arrays of `Proc`s that will evaluate to `true` when the exception should be retried.

## 3.1.1

Expand Down
9 changes: 8 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,9 @@ Here are the available options, in some vague order of relevance to most common
- An `Array` of `Exception` classes (retry any exception of one of these types, including subclasses)
- A `Hash` where the keys are `Exception` classes and the values are one of:
- `nil` (retry every exception of the key's type, including subclasses)
- A single `Proc` (retries exceptions ONLY if it returns truthy)
- A single `Regexp` pattern (retries exceptions ONLY if their `message` matches the pattern)
- An array of patterns (retries exceptions ONLY if their `message` matches at least one of the patterns)
- An array of `Proc` and/or `Regexp` (retries exceptions ONLY if at least one exception matches `Regexp` or the `Proc` evaluates to `true`)
Copy link
Contributor

Choose a reason for hiding this comment

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

grammar: "retries exceptions ONLY if at least one exception message matches a Regexp or at least one Proc evaluates to true" (or "returns truthy"... not sure which it is but you should be correct and consistent in the documentation).



### Configuration
Expand Down Expand Up @@ -132,10 +133,16 @@ end

You can also use a hash to specify that you only want to retry exceptions with certain messages (see [the documentation above](#configuring-which-options-to-retry-with-on)). This example will retry all `ActiveRecord::RecordNotUnique` exceptions, `ActiveRecord::RecordInvalid` exceptions where the message matches either `/Parent must exist/` or `/Username has already been taken/`, or `Mysql2::Error` exceptions where the message matches `/Duplicate entry/`.

A `Regexp` (or array of `Regexp`s). If any of the `Regexp`s match the exception's message, the block will be retried.
A `Proc` (or array of `Proc`s) that evaluates the exception being handled and returns `true` if the block should be retried. If any of the procs in the list return `true`, the block will be retried.
You can also mix and match `Proc`s and `Regexp`s in an `Array`
Copy link
Contributor

Choose a reason for hiding this comment

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

this is all sort of duplicative of the docs above... it's probably better to just link back there than to duplicate the explanation.

Copy link
Author

@rafaelsales rafaelsales Feb 3, 2020

Choose a reason for hiding this comment

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

I'm just staying consistent with the style of this documentation. Above there are other expanded versions of what's on those bullet points as well.

>>>>>>> Accept Proc in the `:on` argument

```ruby
Retriable.retriable(on: {
ActiveRecord::RecordNotUnique => nil,
ActiveRecord::RecordInvalid => [/Parent must exist/, /Username has already been taken/],
ActiveRecord::RecordNotFound => -> (exception, try, elapsed_time, next_interval) { exception.model == User }
Mysql2::Error => /Duplicate entry/
}) do
# code here...
Expand Down
28 changes: 22 additions & 6 deletions lib/retriable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,17 +60,33 @@ def retriable(opts = {})
return Timeout.timeout(timeout) { return yield(try) } if timeout
return yield(try)
rescue *[*exception_list] => exception
if on.is_a?(Hash)
raise unless exception_list.any? do |e|
exception.is_a?(e) && ([*on[e]].empty? || [*on[e]].any? { |pattern| exception.message =~ pattern })
end
end

interval = intervals[index]
raise unless matched_exception?(on, exception, try, elapsed_time.call, interval)

Choose a reason for hiding this comment

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

Line is too long. [88/80]


on_retry.call(exception, try, elapsed_time.call, interval) if on_retry
raise if try >= tries || (elapsed_time.call + interval) > max_elapsed_time
sleep interval if sleep_disabled != true
end
end
end

def matched_exception?(on, exception, *additional_args)

Choose a reason for hiding this comment

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

Method has too many lines. [14/10]

Copy link
Contributor

Choose a reason for hiding this comment

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

trivial but maybe rename additional_args to proc_args? so it's explicit in the naming where these args are going.

i don't love the name proc_args so maybe you can think of something better (matcher_proc_args?), but additional_args kind of makes it seem like the args will be used in the matched_exception? method, which they will not.

Copy link

Choose a reason for hiding this comment

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

Naming/UncommunicativeMethodParamName: Method parameter must be at least 3 characters long.

return true unless on.is_a?(Hash)

on.any? do |expected_exception, matchers|
next false unless exception.is_a?(expected_exception)
next true if matchers.nil?

Array(matchers).any? do |matcher|
if matcher.is_a?(Regexp)
exception.message =~ matcher
elsif matcher.is_a?(Proc)
matcher.call(exception, *additional_args)
else
raise ArgumentError, 'Exception hash values must be Proc or Regexp'
end
end
end
end
private_class_method :matched_exception?
end
28 changes: 28 additions & 0 deletions spec/retriable_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,34 @@ def increment_tries_with_exception(exception_class = nil)
end
end

it '#retriable retries with a hash exception where the value is a proc that returns true' do

Choose a reason for hiding this comment

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

Line is too long. [96/80]

matcher = lambda do |e, _try, _elapsed_time, _next_interval|
e.message == 'something went wrong'
end
tries = 0
expect do
subject.retriable on: { TestError => matcher }, tries: 2 do
tries += 1
raise TestError, 'something went wrong'
end
end.must_raise TestError

expect(tries).must_equal 2
end

it '#retriable does not retry with a hash exception where the value is a proc that returns false' do

Choose a reason for hiding this comment

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

Line is too long. [104/80]

matcher = ->(e, *_args) { e.message == 'something went wrong' }
tries = 0
expect do
subject.retriable on: { TestError => matcher }, tries: 2 do
tries += 1
raise TestError, 'not a match'
end
end.must_raise TestError

expect(tries).must_equal 1
end

it "runs for a max elapsed time of 2 seconds" do
described_class.configure { |c| c.sleep_disabled = false }

Expand Down