Clean up client_getter (and document it) #5
Milestone
Comments
benjaminjkraft
added a commit
that referenced
this issue
Sep 3, 2021
ContextType is in use at Khan as a part of our ka-context system; it basically just lets you configure the type to pass as the `ctx` argument to genqlient helpers (or say to omit such an argument). ClientGetter I wrote thinking we might use it; then we didn't (because we have a few different clients we may use) but it's not much code and may be helpful to others. In this commit I clean up, document, and add tests for both options. The cleanup is mainly for ClientGetter, which was kind of broken before because it was a Go snippet but couldn't specify imports. I was thinking maybe you want to be able to write `ctx.Something()`, but I just don't see how to make it work, so I made it a function of context, which is probably the better idea anyway. Additionally, I improved the documentation for both, and added tests for those and several other config options that weren't completely tested. Fixes #5. Issue: #5 Test plan: make check Reviewers: marksandstrom, miguel, adam
benjaminjkraft
added a commit
that referenced
this issue
Sep 7, 2021
## Summary: ContextType is in use at Khan as a part of our ka-context system; it basically just lets you configure the type to pass as the `ctx` argument to genqlient helpers (or say to omit such an argument). ClientGetter I wrote thinking we might use it; then we didn't (because we have a few different clients we may use) but it's not much code and may be helpful to others. In this commit I clean up, document, and add tests for both options. The cleanup is mainly for ClientGetter, which was kind of broken before because it was a Go snippet but couldn't specify imports. I was thinking maybe you want to be able to write `ctx.Something()`, but I just don't see how to make it work, so I made it a function of context, which is probably the better idea anyway. Additionally, I improved the documentation for both, and added tests for those and several other config options that weren't completely tested. Fixes #5. Issue: #5 ## Test plan: make check Author: benjaminjkraft Reviewers: dnerdy, aberkan, MiguelCastillo Required Reviewers: Approved By: dnerdy Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint Pull Request URL: #77
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
One option is to just say it's a function whose argument is context. (That way we can handle imports right.) But ideally it could also be a global var; and what if you want to be able to return an error?
Or we could just flag it out for now, since Khan didn't end up needing it.
The text was updated successfully, but these errors were encountered: