httpexpect
httpexpect copied to clipboard
gavv
Add Request.WithRetryFunc
coverage: 94.814% (+0.001%) from 94.813% when pulling 539b74e99824287731db3d669ce1210a3f1cd45f on rafiramadhana:427-with-retry-fn into 85c3b02d2e4d884fa345678adc57e775187d9454 on gavv:master.
Thanks for PR!
A few comments:
-
I think WithRetryFunc should override built-in retry policy entirely. i.e., if retryFn is set, it should be used instead of shouldRetry(). Rationale:
- otherwise, the user won't be able to cancel behavior of built-in policies if it's not desired
- also it's not obvious how exactly combined built-in policy + custom retry func behave together (currently code uses logical AND but you can't just guess it, you need to checks docs or source code)
-
After doing this, tests should be updated accordingly I guess.
-
I suggest to rename "WithRetryFunc" to "WithRetryPolicyFunc" and in docs, say that "WithRetryPolicy" is for using one of built-in policies, and "WithRetryPolicyFunc" is for using custom user-defined policy. This way relations of these methods will be more clear.
-
Let's also move WithRetryPolicyFunc right after WithRetryPolicy in the file.
-
Let's make "WithRetryPolicy" and "WithRetryPolicyFunc" mutual exclusive. If user calls both, we'll report a failure. (Please cover it in TestRequest_Conflicts).
-
Could you please also cover invalid usage of WithRetryPolicyFunc in TestRequest_Usage (move nil argument test there) and TestRequest_Order (add new test there)?
-
Let's make examples in readme and comment a bit more illustrative, e.g. it can be
return resp.StatusCode == http.StatusTeapotinstead ofreturn true.
I suggest to rename "WithRetryFunc" to "WithRetryPolicyFunc" and in docs, say that "WithRetryPolicy" is for using one of built-in policies, and "WithRetryPolicyFunc" is for using custom user-defined policy. This way relations of these methods will be more clear.
Let's make "WithRetryPolicy" and "WithRetryPolicyFunc" mutual exclusive.
overall, im agree with your concerns and suggestions
ok, i will do the update, thanks
