feat: Introduce Context #287
Conversation
|
I'm concerned about the duplication in SendRequest/SendRequestWithContext - it might be better to have SendRequest just call SendRequestWithContext with a context.Background(), and then that way you only have one version of the code. |
| @@ -220,6 +222,89 @@ func (c *Client) SendRequest(method string, rawURL string, data url.Values, | |||
| return c.doWithErr(req) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
OK - I still think we should modify SendRequest to have no method body and just instead call
return SendRequestWithContext(context.Background(), method, rawURL, data, headers, body)
| } | ||
| if c.OAuth() != nil { | ||
| oauth := c.OAuth() | ||
| token, _ := c.OAuth().GetAccessToken(context.TODO()) |
There was a problem hiding this comment.
Should pass through the context here as well.
| @@ -25,13 +26,13 @@ func ReadLimits(pageSize *int, limit *int) int { | |||
| } | |||
| } | |||
|
|
|||
| func GetNext(baseUrl string, response interface{}, getNextPage func(nextPageUri string) (interface{}, error)) (interface{}, error) { | |||
| func GetNextWithContext(ctx context.Context, baseUrl string, response interface{}, getNextPage func(ctx context.Context, nextPageUri string) (interface{}, error)) (interface{}, error) { | |||
There was a problem hiding this comment.
I think we need to add this as a second method, similar to the SendRequestWithContext, otherwise we're going to break everyone's clients.
| } | ||
| } | ||
|
|
||
| func (c *RequestHandlerWithContext) sendRequest(method string, rawURL string, data url.Values, |
There was a problem hiding this comment.
I think what you want here is to have the first method here be a context.Context and then you can edit the methods below to call this instead.
| @@ -22,7 +30,16 @@ func NewRequestHandler(client BaseClient) *RequestHandler { | |||
| } | |||
| } | |||
|
|
|||
| func (c *RequestHandler) sendRequest(method string, rawURL string, data url.Values, | |||
| func NewRequestHandlerWithContext(client BaseClient) *RequestHandlerWithContext { | |||
There was a problem hiding this comment.
Not sure you need a separate struct for this honestly, just add a new method/methods on the RequestHandler and call it from everything.
Fixes
Introduce Context
Checklist
If you have questions, please file a support ticket, or create a GitHub Issue in this repository.