More flexible result matching #61
Conversation
|
|
||
| assert elapsed / 1_000 >= 250 | ||
| assert f.() == :error | ||
| assert Regex.scan(~r/running/, capture_log(f)) |> length == 1 |
There was a problem hiding this comment.
Do we really need to capture the log messages here and elsewhere? I personally find it redundant and a bit noisy.
There was a problem hiding this comment.
This was my way of counting the number of executions. Basically just using logs as a side effect that I can count with capture_log. I prefer it over the duration measuring, but I'll use that method if you prefer.
test/retry_test.exs
Outdated
| after | ||
| result -> result | ||
| else | ||
| error -> :not_this |
There was a problem hiding this comment.
Mind fixing the warning(s) by renaming this to _error?
| {%NotOkay{}, NotOkay}, | ||
| {%NotOkay{}, [Foo, NotOkay]}, | ||
| {:not_ok, fn _ -> true end}, | ||
| {:not_ok, [fn _ -> false end, fn _ -> true end]}, |
There was a problem hiding this comment.
While I think this is a great feature, I feel like passing a function or a list of functions to atoms is semantically weird 🤔. The original idea behind atoms is to allowlist things like :error, {:error, foo}, :kaput, etc. Although come to think of it, a more inclusive name for it would be patterns. I think we can add patterns and deprecate atoms if necessary.
Thinking out loud, what if we create a new option called when/when_truthy that accepts one or more functions instead? I personally think it reads better as well, i.e. "retry when a function that accepts the return value evaluates to true".
There was a problem hiding this comment.
I agree that atoms is no longer the appropriate name, but we obviously don't want to remove atoms for backward compatibility reasons so I was hoping we could merge this PR and then maybe in a follow-up add an alias for it and deprecate it.
As far as choosing a new name, I think when or patterns are both fine. Personally I wouldn't add a third parameter for functions separate from one for atoms (not sure if that's what you were suggesting). If anything, I would go the opposite direction and combing the rescue_only functionality in with when (or whatever we call it) so you just have a single parameter for deciding when to retry.
README.md
Outdated
| * The atom is returned in the first position of a two-tuple (for example, `{:not_okay, _}`) | ||
| * A struct of that type is returned/raised | ||
| * The special atom `:all`. In this case, all values/exceptions will be retried. | ||
| * A function (for example: `fn val -> val.starts_with("ok") end`) or partial function (for example: `fn {:error, %SomeStruct{reason: "busy"}} -> true`). The function will be called with the return value and the `do` block will be retried if the function returns a truthy value. If the function returns a falsy value or if no function clause matches, the `do` block will not be retried. |
There was a problem hiding this comment.
Nitpick: val.starts_with("ok") looks more like snake-cased Scala than Elixir 😛. Do you mean String.starts_with?(val, "ok")?
There was a problem hiding this comment.
lol. Yeah my scala background is showing. I'll fix it.
|
How are we feeling about this? Anything I can do to push it forward? |
|
Periodic bump. Any feedback? Can we merge? |
|
Works great, I'm using your branch @nathanalderson , thank you for this patch! |
|
Thanks for merging! |
This PR adds support for more flexible matching on the return value and raised exception for determining whether to retry. Instead of only supporting a list of atoms for each, you can now provide any of the following:
:not_okay,SomeStruct, orCustomError). In this case, thedoblock will be retried in any of the following cases:{:not_okay, _}):all. In this case, all values/exceptions will be retried.fn val -> val.starts_with("ok") end) or partial function (for example:fn {:error, %SomeStruct{reason: "busy"}} -> true). The function will be called with the return value and thedoblock will be retried if the function returns a truthy value. If the function returns a falsy value or if no function clause matches, thedoblock will not be retried.doblock will be retried if any of the items in the list matches.This should address both #54 and #56.