-
Notifications
You must be signed in to change notification settings - Fork 119
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
Remove current_request_steps in Req.Request #255
base: main
Are you sure you want to change the base?
Conversation
Thank you for the PR. It seems we are missing tests because the build should have failed as we needed this design for things like not replying base_url+url on retries, etc. I’ll investigate. Maybe it is not needed after all! |
Sure! Cheers. |
Note to self: revisit Req.Request.prepare/1 while at it. |
b0b1a5d
to
b51f580
Compare
Hi @wojtekmach, I noticed that Line 979 in 7616d39
Then we don't run Line 1587 in 7616d39
In fact, retry does not run any of the input request_steps , as current_request_steps is empty...
This commit "consumes" the Line 975 in 7616d39
Cheers. |
249b80c
to
16991cd
Compare
Sorry for delay, this is tricky because the whole reason we introduced :current_request_steps was to avoid this: Req.new(params: [a: 1])
|> Req.Request.prepend_response_steps(
# as if we don't have current_request_steps
test: fn {req, resp} ->
req = put_in(req.current_request_steps, Keyword.keys(req.request_steps))
{req, resp}
end
)
|> Req.get!(
retry_delay: 10,
plug: fn conn ->
dbg(conn.query_string)
# conn.query_string #=> "a=1"
# conn.query_string #=> "a=1&a=1"
# conn.query_string #=> "a=1&a=1&a=1"
# conn.query_string #=> "a=1&a=1&a=1&a=1"
Plug.Conn.send_resp(conn, 500, "")
end
) (#140) But yes, on the flip side it causes #296. I haven't made up my mind yet which way to go, apologies for the delay. |
Just use request_steps.