Something went wrong on our end. Please try again.
Created by: LawnGnome
This PR moves the apiRequest type and its associated functions in cmd/src out into a new internal/api package.
I'm working on https://github.com/sourcegraph/sourcegraph/issues/12333, and want to keep as much of the business logic of interacting with the campaign backend out of cmd/src as possible: ideally, I want cmd/src to be the "view" layer — handling flags and managing output — and have internal/campaigns be the service layer that implements all the actual logic. I believe this separation will allow for better testing and code quality. (And, if I'm honest, this is also partly because I was starting to have to be very careful about prefixing identifiers in cmd/src, and that was making for seriously unwieldy type and variable names.)
In order to do this, internal/campaigns has to be able to issue GraphQL requests. Just making apiRequest and friends public doesn't totally address this: it sets up potential circular dependencies, and apiRequest is tied to global state via its use of cfg, which makes it hard to reason about, and to mock for future testing purposes as I work on https://github.com/sourcegraph/sourcegraph/issues/12333.
I haven't significantly changed the internals of issuing GraphQL requests: the code in api.Request's methods will look extremely familiar to anyone who's looked at apiRequest.do() previously.
There are a handful of specific changes that I think are improvements, but are open to discussion (just like everything else, of course):
There's no longer a concept of a done field: the request Do methods instead return a bool, error tuple that indicate whether the result was filled out (with the case where it's not being when -get-curl is enabled). This means that functions that make GraphQL requests that previously used done are now written in a more top-down, procedural way, which I think is easier to read, but at the expense of a slightly more complicated if statement to check if err != nil || !ok in many cases.
dontUnpackErrors has been replaced with a parallel set of Do methods: one does the normal default unwrapping of the GraphQL response, and one doesn't. I think this is clearer, but this is certainly a matter of taste.
Contexts are now propagated consistently through the API package, although I haven't done any work to really take advantage of that thus far.
We can now test the API implementation more easily than when it lived in cmd/src, since it no longer relies on global state.
As Client and Request are interfaces, we can easily mock GraphQL requests in other tests within src-cli; I expect to add such a helper as I work on https://github.com/sourcegraph/sourcegraph/issues/12333.
This gives us some options for figuring out what context behaviour we think should be the default when executing src commands: we could now start looking at providing consistent options for timeouts and signal handling.
cc: @chrispine