Skip to content

Conversation

jamsinclair
Copy link

@jamsinclair jamsinclair commented Oct 10, 2016

Was hesitant to re-open the issue you just closed #425. I had a similar situation where I was trying to retrieve a large number or all records via paging and promises.

This PR follows in the footsteps of #262 and adds pluggable promise support to all getPage based methods.

Not as nice as I would've liked. Most of it is duplicated code taken from the #262 solution.

Example outcome from PR:

var GitHubApi = require('github');

var gh = new GitHubApi ({
  Promise: require('bluebird')
});

function getAllOrgRepos(orgName) {
  var repos = [];

  function pager(res) {
    repos = repos.concat(res);
    if (gh.hasNextPage(res)) {
      return gh.getNextPage(res)
        .then(pager);
    }
    return repos;
  }

  return gh.repos.getForOrg({ org: orgName })
    .then(pager);
}


getAllOrgRepos('organization')
  .then(/* Do stuff with all repos */);

If this is not at all how you would like it approached feel free to close this PR, as not to waste your time 😸. Very happy to make changes.

Edit: Tidied up example code.

return callback(new error.NotFound("No " + which + " page found"));
if (!url) {
var urlErr = new error.NotFound("No " + which + " page found");
return self.Promise && !callback ? self.Promise.reject(urlErr) : callback(urlErr);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth throwing an error if neither promise or callback defined, like below?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you. Not a dealbreaker without it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave it be 😄 , I'm sure my lazy developer habits will come back to haunt me.

@kaizensoze
Copy link

kaizensoze commented Oct 10, 2016

This looks good. I'll add the example output to the examples section. As a bonus, how much work would it be to get the promise stuff also compatible with Q? If a significant amount I'll just go ahead and merge this.

@jamsinclair
Copy link
Author

jamsinclair commented Oct 10, 2016

From a quick 5 minute play, it seems like the Q library is compatible. I've commented in the issue #400 😄

If you're fine with the PR, happy for you to merge 👍

@kaizensoze
Copy link

Awesome. Good to go.

@kaizensoze kaizensoze merged commit b29c091 into octokit:master Oct 10, 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.

getNextPage not correctly promisified
2 participants