Skip to content

Conversation

@LawnGnome
Copy link
Contributor

This PR moves the apiRequest type and its associated functions in cmd/src out into a new internal/api package.

Why?

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.

What's changed

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):

  1. 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.

  2. 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.

  3. Contexts are now propagated consistently through the API package, although I haven't done any work to really take advantage of that thus far.

What does this allow in the future

  1. We can now test the API implementation more easily than when it lived in cmd/src, since it no longer relies on global state.

  2. 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.

  3. 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

@LawnGnome LawnGnome added enhancement New feature or request team/code-search labels Aug 7, 2020
@LawnGnome LawnGnome added this to the 3.19 milestone Aug 7, 2020
@LawnGnome LawnGnome self-assigned this Aug 7, 2020
@LawnGnome LawnGnome requested a review from mrnugget August 7, 2020 04:27
@mrnugget mrnugget requested a review from efritz August 7, 2020 08:04
Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for doing this.

Disclaimer: I haven't checked every line here. But I like the overall structure, especially since I could now, theoretically, import this in my test scripts to talk to the API 👍

@efritz
Copy link
Contributor

efritz commented Aug 7, 2020

This is a big change - seems good at a glance but probably won't be able to do an in-depth review. I'm good with this as long as the lsif-upload command still works :p

@LawnGnome
Copy link
Contributor Author

I tested our array of commands before I opened the PR, but I did an extra bonus check of the lsif upload functionality just now to be totally sure. 😃

@LawnGnome LawnGnome merged commit 19718ea into master Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request team/code-search

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants