Add pagination examples for RESTClient and GQLClient #102
Conversation
samcoe
force-pushed
the
pagination-examples
branch
from
425886f to
e8e82da
Compare
example_gh_test.go
Outdated
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
| body, err := io.ReadAll(response.Body) |
There was a problem hiding this comment.
Nit: More recently I started to prefer json.Decoder to decode a Reader right directly into the destination struct without having to first save it to an intermediate buffer. Maybe you can try it here to simplify the example?
example_gh_test.go
Outdated
| requestPath := "repos/cli/cli/releases" | ||
| page := 1 | ||
| hasNextPage := true | ||
| for hasNextPage { |
There was a problem hiding this comment.
Style nit: this could be an endless loop, with an explicit break happening if findNextPage() returned false. Logic would be lightly simpler and you wouldn't have to keep a hasNextPage variable outside of the loop
| var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) | ||
| findNextPage := func(response *http.Response) (string, bool) { | ||
| for _, m := range linkRE.FindAllStringSubmatch(response.Header.Get("Link"), -1) { |
There was a problem hiding this comment.
Thought for the future: having to do this as an extension author feels a bit low-level and requires understanding of certain details in the GitHub API. I wonder if we could provide facilities to encapsulate pagination from go-gh in the future?
| variables := map[string]interface{}{ | ||
| "input": githubv4.AddStarInput{ |
There was a problem hiding this comment.
I'm a bit torn: on one hand I do appreciate that the new example has fewer dependencies, but I also did find value in this example demonstrating the use of the shurcooL library that ensures typed inputs for each GraphQL mutation. I would actually recommend the use of that library to API clients such as extensions. @mntlty: thoughts? #99
There was a problem hiding this comment.
I think there's two use cases of what the examples here are for.
- To show how
go-ghcan be used - To show how
go-ghshould be used
If it's the first, having another package when a struct will do the job isn't necessary. If it's the second, having githubv4 here introduces it to users who will likely benefit from the structure and safety it provides. They don't have to use it, but it's a big help to have the compile time checks.
Personally I learned about githubv4 when contributing to the CLI, which not everyone is fortunate enough to be able to do ❤️
There was a problem hiding this comment.
My thoughts on this are that as go-gh is meant to be used in other projects and all of our dependencies come with it so we should try hard to minimize them. If go had a way to mark a dependency for test or examples only then this wouldn't matter. This was a case I felt that having the dependency was not worth it. What I think would be a good compromise would be to add a comment to this example where the struct is defined saying that the user could alternatively use the githubv4 package where these are already defined. What do people think about that idea?
There was a problem hiding this comment.
@samcoe I like that compromise!
Fixes #100
Additionally I removed our one reference to
github.com/shurcooL/githubv4, it seemed like a large dependency to bring in just for an example.