Skip to content

The gh api command needs some love #10558

@williammartin

Description

@williammartin

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:

cli/pkg/cmd/api/api.go

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    coreThis issue is not accepting PRs from outside contributorsdiscussFeature changes that require discussion primarily among the GitHub CLI teamgh-apirelating to the gh api commandtech-debtA chore that addresses technical debt

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions