-
-
Notifications
You must be signed in to change notification settings - Fork 770
Added AddRetryAfterErrorCondition() option #384
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #384 +/- ##
==========================================
+ Coverage 95.97% 95.99% +0.01%
==========================================
Files 10 10
Lines 1019 1023 +4
==========================================
+ Hits 978 982 +4
Misses 23 23
Partials 18 18
Continue to review full report at Codecov.
|
jeevatkm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gtpaulose Thanks for the PR, surely this handy for the resty users.
Please take a look at a couple of comments.
| // an error from the http response | ||
| func (c *Client) AddRetryAfterErrorCondition() *Client { | ||
| c.AddRetryCondition(func(response *Response, err error) bool { | ||
| if response.IsError() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gtpaulose can you please add a test case for success and failure flow of new condition?
More info: https://codecov.io/gh/go-resty/resty/pull/384/diff?src=pr&el=tree#diff-Y2xpZW50Lmdv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey did you mean the test case for 'refiring under an error and non error scenario (i.e response.IsError() == true and response.IsError() == false )? '
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, ive added the test case. Ive reused an existing path in the createGetServer() switch case, hope thats okay!
|
Is there anymore changes required? |
jeevatkm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gtpaulose Thanks for the updates and for taking care of the review comments.
I'm really sorry for the delay, due to personal reasons.
A really simple option so folks can set the retry options with easy and less verbosity in their code.