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

Conversation

rafaelsales
Copy link

@rafaelsales rafaelsales commented Oct 14, 2017

#18

end

it '#retriable does not retry with a hash exception where the value is a proc that returns false' do
matcher = ->(e, *args) { e.message == 'something went wrong' }

Choose a reason for hiding this comment

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

Unused block argument - args. If it's necessary, use _ or _args as an argument name to indicate that it won't be used.

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]

@@ -313,6 +313,34 @@ class SecondTestError < TestError; end
expect(exceptions[3].class).must_equal StandardError
end

it '#retriable retries with a hash exception where the value is a proc that returns true' do
matcher = ->(e, _try, _elapsed_time, _next_interval) {

Choose a reason for hiding this comment

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

Use the lambda method for multiline lambdas.

@@ -313,6 +313,34 @@ class SecondTestError < TestError; end
expect(exceptions[3].class).must_equal StandardError
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]

lib/retriable.rb Outdated
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.

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]

@rafaelsales
Copy link
Author

@kamui ready!

@rafaelsales
Copy link
Author

Ping!


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

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.

Copy link
Contributor

@apurvis apurvis left a comment

Choose a reason for hiding this comment

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

not sure how much water my opinion carries but in general i think this is a good change.

@apurvis
Copy link
Contributor

apurvis commented Nov 6, 2017

(you could propose the version bump to 3.2.0 on this PR though maybe @kamui just prefers to handle the version bumping himself)

README.md Outdated
- 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).

@jcmfernandes
Copy link

@kamui @rafaelsales what do we need to do here to get this merged? How can I help?

@jcmfernandes
Copy link

Ping ^

lib/retriable.rb Outdated
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)
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.

@rafaelsales
Copy link
Author

@kamui could you take a look at this one?

it "#retriable does not retry with a hash exception where the value is a proc that returns false" do
matcher = ->(e, *_args) { e.message == "something went wrong" }
tries = 0
expect {
Copy link

Choose a reason for hiding this comment

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

Style/BlockDelimiters: Avoid using {...} for multi-line blocks.

e.message == "something went wrong"
end
tries = 0
expect {
Copy link

Choose a reason for hiding this comment

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

Style/BlockDelimiters: Avoid using {...} for multi-line blocks.

@jcmfernandes
Copy link

Thank you so much @rafaelsales! Um abraço amigo de Portugal 🙌 .

@a-tommyyy
Copy link

I hope this feature will be merged.
@kamui

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.

None yet

5 participants