Add Request.WithRetryFunc

[rafiramadhana]

Resolves #427

[coveralls]

coverage: 94.814% (+0.001%) from 94.813%
when pulling 539b74e on rafiramadhana:427-with-retry-fn
into 85c3b02 on gavv:master.

[gavv]

Thanks for PR!

A few comments:

1. 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)

2. After doing this, tests should be updated accordingly I guess.

3. 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.

4. Let's also move WithRetryPolicyFunc right after WithRetryPolicy in the file.

5. Let's make "WithRetryPolicy" and "WithRetryPolicyFunc" mutual exclusive. If user calls both, we'll report a failure. (Please cover it in TestRequest_Conflicts).

6. Could you please also cover invalid usage of WithRetryPolicyFunc in TestRequest_Usage (move nil argument test there) and TestRequest_Order (add new test there)?

7. Let's make examples in readme and comment a bit more illustrative, e.g. it can be return resp.StatusCode == http.StatusTeapot instead of return true.

[rafiramadhana]

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

[rafiramadhana]

@gavv updated, ready to review

[acouvreur]

Any news on this ?