Skip to content

Conversation

@flyinprogrammer
Copy link

Support parsing 'since' from Link header, which enables us to actually paginate through listing Users. This is especially helpful when working with Github Enterprise.

https://developer.github.com/v3/users/#get-all-users

Also, this is kind of a hack since we're overloading the idea of a 'Page' but it does work well with code like this:

opt := &github.UserListOptions{
    Since: 0,
}

// get all pages of results
var allUsers []github.User
for {
    users, resp, err := client.Users.ListAll(opt)
    if err != nil {
        return err
    }
    allUsers = append(allUsers, users...)
    if resp.NextPage == 0 {
        break
    }
    opt.Since = resp.NextPage
}

Also I did not add tests - if we want tests, and we want this code, I can write tests.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@flyinprogrammer
Copy link
Author

I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot
Copy link

CLAs look good, thanks!

@willnorris
Copy link
Collaborator

The problem with parsing since as an int and overloading page in this way is that since is sometimes a timestamp. For example, when listing commits on a repo, listing notifications, and probably others. I don't actually know off the top of my head if those use the rel=next links in the response, but if they did, then this code would have issues (looks like it would just fail silently).

The "right" approach (and I thought we had a long standing issue open for this, but I can't seem to find it), is that we shouldn't be trying to parse values out of the links in the response headers. We should just be treating those URLs as opaque and fetching them directly to get the next page of results. This gets a little messy with knowing exactly what to unmarshal the response as, but it's certainly doable. Though I know this doesn't help you right now.

In the short term, maybe we should just document how to paginate the "list all users" responses? Because the since parameter in that case is defined as the last user ID that you saw, you actually don't even need the rel=next link from the response. Just take the ID of the last user in the previous page, and use it to populate the since value.

gmlewis added a commit to gmlewis/go-github that referenced this pull request Jun 15, 2016
Fixes google#278.

Change-Id: Ia1d0c1f6b395d493714d91e0f6e79dc2506f8fde
gmlewis added a commit to gmlewis/go-github that referenced this pull request Jun 16, 2016
Fixes google#278.

Change-Id: Ia1d0c1f6b395d493714d91e0f6e79dc2506f8fde
@gmlewis gmlewis closed this in #367 Jun 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants