Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Allow page size option (per_page) in listPullRequestFiles call #408

Open
rausanka opened this issue Dec 17, 2016 · 3 comments

Comments

@rausanka
Copy link

The listPullRequests call allows an optional second parameter for options that include setting the page size for for a request. GitHub allows per_page to also be sent along for the call wrapped by listPullRequestFiles but this library doesn't allow options to be passed.

I'm happy to submit a PR but want to know:

  1. If you're cool with this
  2. How you'd like me to add this functionality without breaking any existing uses of the library that rely on the second parameter being a callback. implement a listPullRequestFilesWithOptions?
@clayreimann
Copy link
Member

clayreimann commented Dec 19, 2016

I for one am cool with this idea. But, are you sure we don't just fetch all pages by default? Many of our methods fetch all of the pages behind the scenes.

We probably want to re-think how we implement paged methods in general.

@rausanka
Copy link
Author

Based on https://github.com/github-tools/github/blob/master/lib/Repository.js, it seems that the only Repository call that requests all pages is listProjects. Also, it doesn't look like the use of _requestAllPages in listProjects takes advantage of using the per_page option to increase page sizes from 30 to 100.

Given this, it seems like there are a few possible improvements (not necessarily mutually exclusive):

  1. Change all of the paginated calls in Repository.js to use _requestAllPages instead of _request
  2. Change listProjects to set per_page to 100 to reduce necessary requests to get all pages
  3. To your point about re-thinking how paged methods are implemented, maybe have two calls for each method, one for getting individual pages, one for getting all pages (e.g. listPullRequestFiles for getting individual pages as before and a new listAllPullRequestFiles for recursively getting all files across all pages).

@rausanka
Copy link
Author

Been thinking about this more recently and I think we should definitely do #2 above and I'd prefer #3 over #1 as large repositories might have many pages (e.g. at QuanticMind we're creating ~300 PRs/month) that will quickly get you into GitHub API rate limiting hell. Thoughts?

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

No branches or pull requests

2 participants