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

Consolidated `retry/2` by moving `exceptions` argument into `rescue_only` option #12

Merged
merged 3 commits into from Aug 27, 2017

Conversation

Projects
None yet
3 participants
@fredwu
Copy link
Contributor

fredwu commented Aug 26, 2017

Hi,

#11 introduced retry/3 to specifically cater for a list of exceptions to allow rescuing. It's great but it means having both retry/2 and retry/3. I propose to have only retry/2 and have exceptions be passed in as an option. This would also allow more flexible expansion of options in the future.

What do you guys thinks? @safwank @neerfri

fredwu added some commits Aug 26, 2017

@fredwu

This comment has been minimized.

Copy link
Contributor Author

fredwu commented Aug 26, 2017

On a somewhat unrelated note, the test suite takes over 20 seconds to run due to the timing checks. In cd39ed8 I reduced those timing numbers so now it runs under 3 seconds.

@safwank

This comment has been minimized.

Copy link
Owner

safwank commented Aug 26, 2017

@fredwu Ah, that's actually pretty nice. Why didn't I think of that earlier?

I'll sleep on it tonight.

@safwank safwank merged commit 07c14cb into safwank:master Aug 27, 2017

@halostatue

This comment has been minimized.

Copy link

halostatue commented Aug 28, 2017

You probably want to consider a slight change to this because it is possible to call retry without a with: parameter now. The simplest change would be to use Keyword.fetch!(opts, :with) instead of opts[:with], but by using a function and two heads on the macro, you can get the compile-time failure that originally existed:

    stream_builder = opts[:with]
    exceptions     = opts[:rescue_only] || [RuntimeError]

    quote do
      fun = unquote(block_runner(block, exceptions))

      unquote(delays_from(stream_builder))
      |> Enum.reduce_while(nil, fn(delay, _last_result) ->
        :timer.sleep(delay)
        fun.()
      end)
      |> case do
        {:exception, e} -> raise e
        result          -> result
      end
    end
  end
@safwank

This comment has been minimized.

Copy link
Owner

safwank commented Sep 9, 2017

@halostatue Very good point. Something along these lines?

  defmacro retry(opts = [with: _stream_builder], block) do
    do_retry(opts, block)
  end
  defmacro retry(opts = [with: _stream_builder, rescue_only: _exceptions], block) do
    do_retry(opts, block)
  end
@safwank

This comment has been minimized.

Copy link
Owner

safwank commented Sep 9, 2017

Actually there's a more generic way:

  defmacro retry(opts = [{:with, _stream_builder} | _], block) do
    do_retry(opts, block)
  end

That way we not only get compile-time checking for with but also allow other options to be specified without having to add additional function heads. The fact that with has to be the first option is a plus.

@safwank

This comment has been minimized.

Copy link
Owner

safwank commented Sep 10, 2017

Released as v0.8.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment