Alternative go-github API

go-github-client.go

func main() {

  watchers, pager, _, err := client.Activity.ListWatchers("o", "r")
  if pager.HasNext() {
    watchers, pager, _, err = pager.Next()
  }
  //..
  //or:
  watchers, pager, _, err = pager.Last()
}

I'm very hesitant to add yet another return value from these functions... I really hated even adding Response, since in most cases it gets ignored (as you have in your example above). However, one of the primary reasons for returning the Response is because it carries the pagination data. I'd much rather see a method that uses the existing return values to ease pagination.

That said, I don't think handling the actual paging is the difficult part. For example, here is some code I have for walking though all of the repos on an account. I return a channel of github.Repository structs so that I can start processing results immediately, even while it's continuing to fetch additional pages from the API. You'll notice that the bulk of the code is related to the channel, not actually paginating through the github API calls. I'm not sure that a pager object would really make this code that much simpler.

// getRepos retrieves all repositories for the specified owner.
func getRepos(owner string) (<-chan github.Repository, <-chan error) {
    ch := make(chan github.Repository, listPerPage)
    errch := make(chan error)

    go func() {
        opt := &github.RepositoryListByOrgOptions{
            ListOptions: github.ListOptions{PerPage: listPerPage},
        }
        for {
            repos, resp, err := client.Repositories.ListByOrg(owner, opt)
            if err != nil {
                errch <- err
                break
            }

            for _, r := range repos {
                ch <- r
            }

            if resp.NextPage == 0 {
                close(ch)
                break
            }
            opt.ListOptions.Page = resp.NextPage
        }
    }()

    return ch, errch
}

And here's how I call that function...

ch, errch := getRepos(*org)

for {
    select {
    case repo, ok := <-ch:
        if !ok { // channel has closed
            return
        }

        // do something with repo
    case err := <-errch:
        fatalf("%v", err)
    }
}

I believe this was the discussion that led to the current design: google/go-github#22

Thanks for you reply. I wonder if adding Next(), Last() etc convenience methods to Response would be a good idea?

BTW. Usage chan is another thing I had in in mind as a way of achieving asynchrony. But I figured (as even you have pointed out) besides making the code complex, it would also make the API a bit less 'clean' with channels being being passed around.

what would said Next() and Last() functions return?

I slept over this and I think I have more clarity regarding what I was trying to get to.

how about something like this:

iter, err := client.Activity.ListStarrers("o", "r")

// Fetch current result set:
data := iter.Data()

// Move to next page:
iter.Next()
data, err = iter.Data()

// or..
iter.Prev()
data, err = iter.Data()

// Optionally...
iter.JumpTo(5)

Essentially, iter.Next() would call the API method (in this case Activity.ListStarrers) with the correct page number set.

I think this useful because:

• User of the API doesn't need to track which page number he is currently on. The 'iterator' does that.
• If Github later move away from a linear integer based paging to hash based one, API wouldn't change
• It is very rare that the API user would know the exact page number. Most use cases would involve going to next page or previous one. And that needs to be made simpler.
• Return type (atleast in terms of interface) for all methods returning list would become consistent

Infact, the iter.JumpTo could look something like this:

func (i *Iterator ) JumpTo(pg interface{}) {
   // Use stringer
}

okay, but what does func (i *Iterator) Data() return? What I'm getting at, is that I suspect the only thing that would really work is to return interface{}, which requires that the user then type switch to whatever the real type is and you lose all type safety.

One alternative would be to have a new Iterator type for every data type you need to page through, so that Data() can return an exact type rather than iterator{}, but now you've close to doubled the number of types that users of the library have to contend with.

Drats. Its times like these that I feel really really stupid.

I clearly didn't think this through and wasted your time as well.
Although I would blame excessive usage of dynamically typed languages for this stupid suggestion :P

Thanks for your time! And thanks for merging the PR!

No worries... it's not stupid at all. I'm still getting used to working in Go myself... some things really are different here than how you would approach them in other languages (particularly dynamically typed languages, as you mention).

And to be clear, I have no problem with adding something to the library that makes working with paginated results a little easier. I just honestly don't know what else we can do than what we currently have.