Add HTTPClient interface to Pusher struct #559
Conversation
Signed-off-by: NghiaLT <nghialt.11@gmail.com>
|
I like the idea in general. I will give this a proper review ASAP. |
There was a problem hiding this comment.
Thanks for the PR and my apologies for the delayed review. Code-wise, this looks good. I'd just suggest to not create a separate file for a one-method interface. There are also some naming and documentation changes that should help the user to understand what's going on.
prometheus/push/http.go
Outdated
| import "net/http" | ||
|
|
||
| // HTTPClient is a interface for http client | ||
| type HTTPClient interface { |
There was a problem hiding this comment.
- Let's not create a new file for a one-method interface. This interface is tightly coupled to the
Pushertype. Let's put it into the same file. - Let's call it
HTTPDoeras it doesn't really represent a fully fledgedhttp.Client. - Suggested doc comment to tell the reader why we are doing all of this:
// HTTPDoer is an interface for the one method of http.Client that is used by Pusher.
prometheus/push/push.go
Outdated
| @@ -170,7 +170,7 @@ func (p *Pusher) Grouping(name, value string) *Pusher { | |||
|
|
|||
| // Client sets a custom HTTP client for the Pusher. For convenience, this method | |||
| // returns a pointer to the Pusher itself. | |||
| func (p *Pusher) Client(c *http.Client) *Pusher { | |||
| func (p *Pusher) Client(c HTTPClient) *Pusher { | |||
There was a problem hiding this comment.
The doc comment needs to be amended. Suggestion for additional text:
Pusher only needs one method of the custom HTTP client: Do(*http.Request). Thus, rather than requiring a fully fledged http.Client, the provided client only needs to implement the HTTPDoer interface. Since *http.Client naturally implements that interface, it can still be used normally.
Signed-off-by: NghiaLT <nghialt.11@gmail.com>
|
Agreed with your comments! I have updated accordingly, please check @beorn7. |
|
Many thanks. Merging now. |
|
by the way, when will the next release be @beorn7? |
Hi beorn@soundcloud.com,
I have a need to customize HTTP client but the lib only accept http.Client so I made this PR. Please review and comment.
Regards,
NghiaLT.