Closed am-kantox closed 3 years ago
Thanks for the PR @am-kantox. I'll check it out this weekend.
This looks good, but I'm thinking we can make it a bit clearer.
What do you think of specifying acc_name
and acc_initial
in a Keyword list?
retry_while with: linear_backoff(50, 1) |> take(5), accum: [init: 0, name: :count] do
{:cont, count + 1}
end
IMO it makes the arguments a bit more obvious, not to mention it's more consistent with the options
idiom in Elixir. The latter also makes it possible to extend the existing retry_while
to accept the accumulator as an optional argument instead of adding a new function that does something slightly different.
Honestly, I could not disagree more.
The accumulator is not another option by any means and it never gets passed through options (see Enum.reduce/3
and family.)
Passing it as an option would make the whole approach less clean, because when there won’t be any clear distinction between retry_while/2
with or without the accumulator. Also, it’ll make a codebase more cumbersome (although not too much.)
Of course, the library is yours and if you still insist on breaking the Elixir convention of passing an accumulator as a separated argument in favour of passing it as an option, no problem, I can provide the requested changes, but, please, think twice.
OTOH, we might follow for/1
’s approach with into
option and hardcoded acc
name for the accumulator when into:
is presented. I’d vote for that, WDYT?
I think I'm with you on the suggestion the accumulator shouldn't be an option per se as it's not consistent with Enum
functions that deal with accumulators, but I also think passing an atom to represent a variable for the accumulator is a bit weird (would be keen to know if there are examples of something similar being used elsewhere), not to mention the compiler emits a warning for the test complaining variable "count" is unused
.
The into:
option similar to what's used in Elixir comprehensions sounds interesting. Mind showing a concrete example of how this would look?
Also, what do you think of passing the accumulator as an actual variable in the do
block, e.g.
retry_while 0, with: linear_backoff(50, 1) |> take(5) do
count -> {:cont, count + 1}
end
I'm not convinced, but at least it's not throwing a warning 😄.
Also, what do you think of passing the accumulator as an actual variable in the
do
block, e.g.
This is exactly how for/1
comprehension looks like, so I am opt-in for that. In this case, though, we should probably mimic for/1
fully:
retry_while acc: 0, with: linear_backoff(50, 1) |> take(5) do
acc -> {:cont, acc + 1}
end
If acc
option is passed, we expect acc -> ...
notation and provide a meaningful long error message otherwise.
Shall I update the PR with this approach?
Love it. Yes please!
Here we go.
Indeed; bypassing macro hygiene has survived several refactors during which it became obsolete.
Applied everything.
Th PR is inspired by this SO question.
In some cases, subsequent retries in
reduce_while/2
might depend on the previous outcome. In such a case, it’d be great to pass the accumulator through instead of just discarding it. This PR providesreduce_while/3
allowing this functionality.Example