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

switch to :retry 2-arity fun, add support for {:delay, milliseconds} #226

Merged
merged 7 commits into from
Aug 25, 2023

Conversation

DaTrader
Copy link
Contributor

As noted here #203 (comment), I made the agreed changes except for the one changing the behavior of the function when it returns true. That part I left as it was i.e. it will have delay fetched from the retry-after header only if the status code is 429. Again, my advice is not to force users to double check their existing code base when switching to the 2-arity function.

lib/req/response.ex Outdated Show resolved Hide resolved
lib/req/steps.ex Outdated
{request, response_or_exception}
case retry do
{:delay, delay} ->
unless Req.Request.get_option(request, :retry_delay) do
Copy link
Owner

Choose a reason for hiding this comment

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

please change to if/else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but why?

Copy link
Owner

Choose a reason for hiding this comment

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

lib/req/steps.ex Outdated
end

defp apply_retry(fun, request, response_or_exception) do
case Function.info(fun, :arity) do
Copy link
Owner

Choose a reason for hiding this comment

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

I believe Function.info is slow, please use:

fun when is_function(fun, 1) ->
  # ...

fun when is_function(fun, 2) ->
  # ...

@wojtekmach
Copy link
Owner

Sorry about the conflicts, I tried resolving some but not sure if I didn’t make things worse.

@DaTrader
Copy link
Contributor Author

Sorry about the conflicts, I tried resolving some but not sure if I didn’t make things worse.

You removed my tests and on my computer the tests don't pass any more.

@DaTrader
Copy link
Contributor Author

DaTrader commented Aug 25, 2023

It's funny that your CI is passing.

EDIT:
So, what now?

@wojtekmach
Copy link
Owner

If the production code is roughly what you wanted I can take it from here but of course if you’d like to keep going that’s appreciated too. I think I did a merge with GitHub UI so your changes should be recoverable. Sorry about that.

@DaTrader
Copy link
Contributor Author

I'm not following you. What should we do now? Should I bring my tests back or? It's not a problem, just tell me what you expect me to do.

@wojtekmach
Copy link
Owner

I didn’t mean to remove anything it was a botched merge. Sorry for interfering. Please proceed as you see fit and I’ll review it in a couple days.

@DaTrader
Copy link
Contributor Author

Please proceed as you see fit and I’ll review it in a couple days.

No reason to wait. I just commented a single test that you added at the last moment and it all works now. Please merge and then you can work on that test later.

@wojtekmach wojtekmach merged commit 7b0286f into wojtekmach:main Aug 25, 2023
1 of 2 checks passed
@wojtekmach
Copy link
Owner

Thank you!

1 similar comment
@DaTrader
Copy link
Contributor Author

Thank you!

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.

2 participants