search

adlio / trello

Trello API wrapper for Go
MIT License
220 stars 71 forks source link

Allow getting a new client with a context.Context #8

Closed ernesto-jimenez closed 6 years ago

ernesto-jimenez commented 6 years ago

Context is great to allow for cancellation of requests.

With this change it will be possible to get a new client which will set a different context for all its requests.

This solution is backwards-compatible. However, since this package always embeds itself to provide methods like *Board.GetLists(). The current solution could lead to memory leaks or using old contexts if the user is holding the responses in memory.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.3%) to 70.072% when pulling 75f1ba3359fafe59fb6592c57c42cd3066bb8d28 on ernesto-jimenez:context-enabled into d2192fca4c098a2359d6cdea344c667fcfe2628c on adlio:master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.3%) to 70.072% when pulling 75f1ba3359fafe59fb6592c57c42cd3066bb8d28 on ernesto-jimenez:context-enabled into d2192fca4c098a2359d6cdea344c667fcfe2628c on adlio:master.

ernesto-jimenez commented 6 years ago

@adlio what is your plan to deprecate support for old Go versions?

It is possible to work around the current failure with some build tags. But I'm wondering whether it's worth it. The last three major versions have been out for over a year and a half.

adlio commented 6 years ago

@ernesto-jimenez I'm fine with deprecating now, assuming we maintain backwards compatibility with the trello package API, which your change does. Want to update the PR with travis config changes?

I'm still considering the implications of client/context re-use.