cli / go-gh

A Go module for interacting with gh and the GitHub API from the command line.
https://pkg.go.dev/github.com/cli/go-gh/v2
MIT License
322 stars 45 forks source link

Add context to possibly long-running methods (external API) #47

Closed mszostok closed 2 years ago

mszostok commented 2 years ago

Describe the feature or problem you’d like to solve

Currently, there is no option to cancel the ongoing request.

Here is a small example of pagination request where this would be helpful https://gist.github.com/mszostok/b68ff95f85d4b4ff8a27aeed56f9d3ca.

This only fetches GitHub stars, so the payload is not heavy, however if you want to fetch all issues then you have even a bigger problem.

Proposed solution

How will it benefit CLI and its users?

Additional context

There are at least two options how it can be achieved:

  1. Add new methods with the WithContext suffix. For example:

    // GQLClient is the interface that wraps methods for the different types of
    // API requests that are supported by the server.
    type GQLClient interface {
      // Do executes a GraphQL query request.
      // The response is populated into the response argument.
      Do(query string, variables map[string]interface{}, response interface{}) error
    
      // DoWithContext executes a GraphQL query request.
      // The response is populated into the response argument.
      DoWithContext(ctx context.Context, query string, variables map[string]interface{}, response interface{}) error
    }

    Similar approach as Go has for http.Request. It's not a breaking change.

  2. Change all func signature and add the context.Context as a first parameter. It's a breaking change in external API.

There is also an option to add context.Context to struct but it won't work in this case and it's ugly and problematic.

If you agree with one of the provided option, I can create a dedicated PR 👍

mislav commented 2 years ago

@mszostok Thanks for the feature request!

I liked the option (1). @samcoe thoughts?

samcoe commented 2 years ago

@mislav I was just about to comment the same thing. Option 1 sounds like the right approach here.

mszostok commented 2 years ago

Hi,

I prepared a PR that adds context as described in Option 1, see https://github.com/cli/go-gh/pull/50

Cheers!