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

feat: add support for should_retry with arity of 2 to Retry middleware #608

Merged

Conversation

mdlkxzmcp
Copy link
Contributor

@mdlkxzmcp mdlkxzmcp commented Aug 16, 2023

Closes #578

I also formatted the middleware.ex as it caused the lining test to fail aaaand made a typo in the branch name 🤦

I am open to ideas on how to make this work with what is described in that issue... I needed a way to prevent Retry from repeating POST/PUT requests on timeouts, so I wrote it and at first glance thought it was the same issue, but now I see it isn't :)

Edit: with the addition of the suggested changes by @yordis, this now does in fact close the aforementioned issue 🎊

lib/tesla/middleware/retry.ex Outdated Show resolved Hide resolved
lib/tesla/middleware/retry.ex Show resolved Hide resolved
Comment on lines +53 to +55
- `:should_retry` - function with an arity of 1 or 3 used to determine if the request should
be retried the first argument is the result, the second is the env and the third is
the context: options + `:retries` (defaults to a match on `{:error, _reason}`)
Copy link
Member

Choose a reason for hiding this comment

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

the best way to document such complexity is by having a @type maybe called should_retry_func and document it there.

You do not have to do it, just commeting so that in the future others could find this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to do something like that, but I haven't seen anything of the sort in any of the other middlewares... If you want, I can make a follow-up PR that documents all the options. The question is should I leverage typedocs and remove the ## Options section in favor of using something like:

@typedoc "Supported options."
@type options :: %{optional(:delay) => delay(), ...}

@typedoc "The base delay in milliseconds (positive integer, defaults to 50)"
@type delay :: pos_integer()

And then just reference it with t:options/0 in the moduledoc?

Copy link
Member

Choose a reason for hiding this comment

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

I tend to say yes, lets give it a try

@yordis
Copy link
Member

yordis commented Aug 19, 2023

@teamon going to wait for your feedback just in case, but thus far, I am OK with the addition.

@yordis yordis merged commit 9c6d321 into elixir-tesla:master Sep 10, 2023
5 checks passed
@mdlkxzmcp mdlkxzmcp deleted the feat/should-retry-wtih-two-arity branch September 11, 2023 11:26
@mdlkxzmcp mdlkxzmcp restored the feat/should-retry-wtih-two-arity branch September 11, 2023 13:49
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.

Retry middleware: Is there a way to know which request is being retried?
2 participants