-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Description
Description
There are a number of open issues relating to the gh api command. Some of these are small lifts, and others are significant.
As a maintainer of gh, I'm no longer happy accepting changes for gh api due to the state of the code. It is hard to modify the code, it is hard to have confidence in the modifications, and the only really viable options for modification are continuing to add more scattered and entangled conditional behaviour. For example, search for all the places in which isGraphql is mentioned, or look at the logic to compose various readers to manage --jq, --template, --paginate. --slurp:
Lines 477 to 510 in fc19ff3
| var bodyCopy *bytes.Buffer | |
| isGraphQLPaginate := isJSON && resp.StatusCode == 200 && opts.Paginate && opts.RequestPath == "graphql" | |
| if isGraphQLPaginate { | |
| bodyCopy = &bytes.Buffer{} | |
| responseBody = io.TeeReader(responseBody, bodyCopy) | |
| } | |
| if opts.FilterOutput != "" && serverError == "" { | |
| // TODO: reuse parsed query across pagination invocations | |
| indent := "" | |
| if opts.IO.IsStdoutTTY() { | |
| indent = ttyIndent | |
| } | |
| err = jq.EvaluateFormatted(responseBody, bodyWriter, opts.FilterOutput, indent, opts.IO.ColorEnabled()) | |
| if err != nil { | |
| return | |
| } | |
| } else if opts.Template != "" && serverError == "" { | |
| err = template.Execute(responseBody) | |
| if err != nil { | |
| return | |
| } | |
| } else if isJSON && opts.IO.ColorEnabled() { | |
| err = jsoncolor.Write(bodyWriter, responseBody, ttyIndent) | |
| } else { | |
| if isJSON && opts.Paginate && !opts.Slurp && !isGraphQLPaginate && !opts.ShowResponseHeaders { | |
| responseBody = &paginatedArrayReader{ | |
| Reader: responseBody, | |
| isFirstPage: isFirstPage, | |
| isLastPage: isLastPage, | |
| } | |
| } | |
| _, err = io.Copy(bodyWriter, responseBody) | |
| } |
In particular, conditionals like this become an exercise in holding state mentally:
if isJSON && opts.Paginate && !opts.Slurp && !isGraphQLPaginate && !opts.ShowResponseHeaders { It's no one's fault, it's just a result of behavioural accretion. As Sandi Metz describes, the easiest path is just to find the right spot and put a new conditional in there, but at some point we have to pay the piper!
I suspect the primary issue is in having a single code path attempt to address both GraphQL and REST requests, though I'm sure there are others. I could well be wrong though! I would be interested in seeing a much earlier branching, after which only one of these is being considered at a time.
I'm not sure when we will get round to addressing this; it would most likely be motivated by product needs. However, I'm creating this issue as a place that we can discuss, and that can be pointed at if there are more requests for gh api changes.