Skip to content

#569 tamer1an fix for get all pages in _requestAllPages #575

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

Merged

Conversation

tamer1an
Copy link
Contributor

Fixing for #569 _requestAllPages

#569

@tamer1an tamer1an changed the title #569 tamer1an fix request all pages #569 tamer1an fix for get all pages in _requestAllPages Jul 20, 2019
log(`getting next page: ${nextUrl}`);
return this._requestAllPages(nextUrl, options, cb, results);
if(nextUrl) {
options.page = parseInt(
Copy link
Member

Choose a reason for hiding this comment

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

It's possible for options to be undefined. Please handle this prior to setting page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will do that

return this._requestAllPages(nextUrl, options, cb, results);
if(nextUrl) {
options.page = parseInt(
nextUrl.match(/(&page=[0-9]*)/g)
Copy link
Member

Choose a reason for hiding this comment

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

This only matches against the page having a parameter before it. Something like this would break it:

https://api.github.com/organizations/17532995/teams?page=2

Is there a reason we need to match against the &?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will double check

@j-rewerts j-rewerts merged commit 2b6db00 into github-tools:master Aug 25, 2019
@tamer1an tamer1an deleted the #569-tamer1an-fix-requestAllPages branch August 26, 2019 22:17
@tamer1an
Copy link
Contributor Author

tamer1an commented Aug 26, 2019

Congratz with a new version 💪

Thanks for help with PR @j-rewerts 💚 I almost introduced that fix with & but was at holidays

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.

2 participants