-
Notifications
You must be signed in to change notification settings - Fork 240
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
Prepare internals for exposing context.Context in exported API #266
Conversation
Sorry for this PR being so large. Unfortunately, this one needs to be big because I was changing the method signatures for the things that all the other methods call. So that required touching most files in one-shot to avoid breaking the build. The ones where we expose this via the exported API can be done in much smaller chunks to make reviewing much easier. |
As of today there is not a way for callers to control the timeout of requests on a per-call basis, as we do not expose context.Context in the APIs of this package. This change updates some of the internal code paths to use context.Context, making use of a context.TODO() for now (until we can expose context.Context) in the API. This change also includes some style changes to help make the code a bit less dense / more idiomatic in some cases. This also adds a few TODO comments, as a reminder to come back and look at concerning blocks of code that caught my eye.
b2488db
to
bdf4bc5
Compare
I reduced some of the changes I made in this PR since first raising it, and so I'd feel comfortable merging this change without a review. For context, I don't anticipate that this change is going to be contentious as it's just adding |
Just to be clear, I can't merge without a review but the changes seem innocuous enough that I would if I could. |
@stmcallister I noticed you didn't 👀 this PR, and wasn't sure if you have time to review this. If you don't have time would you be willing to give me a blind-trust Approval so I can merge after I fix the conflict (I do have merge access, just can't merge my own PRs as we require a review before merging and I'm not a repo admin). 😄 |
Oh, you just did it; heh. 🤜 🤛 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this! Thanks for doing this work! 👍
As of today there is not a way for callers to control the timeout of requests on
a per-call basis, as we do not expose context.Context in the APIs of this
package.
This change updates some of the internal code paths to use context.Context,
making use of a context.TODO() for now (until we can expose context.Context) in
the API.
This change also includes some style changes to help make the code a bit less
dense / more idiomatic in some cases. This also adds a few TODO comments, as a
reminder to come back and look at concerning blocks of code that caught my eye.