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