add configurable HTTP client via functional options #6
Conversation
- Add WithHTTPClient option for custom http.Client injection
- Add WithRetries option for configurable retry count (default: 2)
- Convert send() to receiver method to access Batcher config
- Replace ioutil.ReadAll with io.ReadAll
- Replace interface{} with any
- Fix defer inside loop anti-pattern
- Fix PostEZCountTime docstring
AObuchow
left a comment
AObuchow
left a comment
There was a problem hiding this comment.
LGTM :), my comments are just nits from previously existing code.
stathat/batcher.go
Outdated
| // If not provided, defaults to 2. | ||
| func WithRetries(n int) Option { | ||
| return func(b *Batcher) { | ||
| if n > 0 { |
There was a problem hiding this comment.
Nit: Could we not allow setting 0 retries, i.e. if n >= 0 {...} ?
There was a problem hiding this comment.
pre-existing default behavior was hardcoded 2 attempts, I believe. Retry logic can easily be updated to be >= (and also updated in the b.send() method to actually act as retries, rather than attempts. Good call.
stathat/batcher.go
Outdated
| for i := 0; i < retries; i++ { | ||
| resp, err := http.DefaultClient.Do(req) | ||
| func (b Batcher) send(req *http.Request) { | ||
| for i := 0; i < b.retries; i++ { |
There was a problem hiding this comment.
Nit: Isn't this technically "attempts" not "retries"? E.g. if b.retries == 1 then we only make a single request attempt, we don't actually retry it one time.
To be more accurate, the for loop should be i <= b.retries or we should rename retries to attempts (though that's less standard).
There was a problem hiding this comment.
Agreed - will update (and see other response as well)
- Allow setting 0 retries via `n >= 0` validation (single attempt) - Change default from 2 to 1 to preserve existing behavior (2 total attempts) - Rewrite send() with idiomatic infinite loop pattern - Update doc comment to clarify: retries=0 means no retries (1 attempt), retries=N means N retries (N+1 total attempts)
Changes
NewBatcher:WithHTTPClient: custom HTTP clientWithRetries: configurable retry countWithEZKey: override EZ key after constructionVerification