Skip to content
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

Support for Request Contexts #64

Open
harmonherring opened this issue Apr 12, 2024 · 3 comments
Open

Support for Request Contexts #64

harmonherring opened this issue Apr 12, 2024 · 3 comments

Comments

@harmonherring
Copy link
Contributor

It doesn't look like any of these requests support caller-defined contexts. @KnutZuidema would you support it if I opened a PR that adds ...WithContext functions for all existing API requests?

For example, I'd add GetByAccountIDWithContext(ctx context.Context, ...), GetByPUUIDWithContext(ctx context.Context, ...) etc etc for summoner routes.

I'm thinking I'd bubble that context down to a duplicate Client.DoRequestWithContext(ctx context.Context, ...) that adds a context to the request with request.WithContext

@KnutZuidema
Copy link
Owner

@harmonherring makes sense to add these. However, instead of creating new methods next to the existing ones, I would bump the major version of the library instead and replace/update the existing methods.

@happsie
Copy link
Contributor

happsie commented Jun 22, 2024

@KnutZuidema in regards to moving to a v2 API, is that something you feel like you want to do or could we help with this? Having a request context can be very good since this library is interacting with third party. Don't want to waste resources/potential rate limiting.

Just wanted to ask since I feel that it will be a pretty big bump in the library and maybe you'd like control of that

@happsie
Copy link
Contributor

happsie commented Sep 16, 2024

Adding to this. Maybe it would be possible to add the option pattern to solve this as well without breaking the backwards compatability of the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants