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

Introduce request contexts #90

Merged
merged 10 commits into from
May 1, 2020
Merged

Introduce request contexts #90

merged 10 commits into from
May 1, 2020

Conversation

weppos
Copy link
Member

@weppos weppos commented Apr 28, 2020

This PR is an initial effort to introduce Go contexts.

This is something I've been planning for quite a long time. An initial request along with a great PR was provided in #82.

I went back and forth about the idea of merging #82 vs making more structural changes. The main benefit of #82 were:

  • reduced impact on the current codebase
  • no need to change current methods signature

The downside were mostly two, related to performances and long-term maintainability:

  • The context is assigned (and stored) into the client, which requires to create copies of the client to use different contexts
  • I'm seeing all clients moving towards per-request signatures, so in the long run this seems to be the direction.

Given that we're not 1.0 yet, I decided to give it a try to use per-request contexts. As mentioned, this is what most clients are moving towards, at least by looking at some quite relevant Go SDKs.

For instance, Cloudflare client is using contexts in new functions. Interestingly, they also have several context-specific methods like ListZonesContext, probably because they have to support both for legacy purposes. That's what I'd like to avoid long terms by eating the bullet sooner rather than later.

Another client that I often observe is github-go, which had some notable influence to our clients. They made a drastic switch google/go-github@23d6cb9 and they call it done. Same story, they went for per-function context.

So here's the changes I made in this PR:

  1. NewRequest and Do are now not exported. This ensures we are free to move things internally without affecting customers outside. I originally kept these names public as I was inspired by go-github, but also because I used them as helpers to test not implemented methods in the context of the client. So I decided to provide a more high level Request method that can be used for this purpose, without allowing anymore to interact with the low-level client internals anymore.
  2. I kept newRequest and Do -> request around to segment the request preparation. It made it easier to test it, also it required minimum changes in our test suite.
  3. I'm propagating the context all the way to the final functions, and at that point I'll see if making them available in the signature or (alternatively) we can also preserve the per-client idea proposed in Add context support to clients #82.

@weppos weppos added the enhancement New feature, enhancement or code changes, not related to defects label Apr 28, 2020
@weppos weppos self-assigned this Apr 28, 2020
@weppos
Copy link
Member Author

weppos commented Apr 28, 2020

Blocked by #91. I've changed the signature of most methods, and I will have to change it again to include the context. So I'm waiting to avoid conflicts.

@weppos weppos added the ready-for-review Pull requests that are ready to be reviewed by other team members. label Apr 30, 2020
@weppos weppos requested a review from aeden April 30, 2020 20:58
@weppos weppos merged commit 5e58562 into master May 1, 2020
@weppos weppos deleted the feature-context branch May 1, 2020 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, enhancement or code changes, not related to defects ready-for-review Pull requests that are ready to be reviewed by other team members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants