Skip to content

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

Merged
j-rewerts merged 3 commits into
github-tools:masterfrom
tamer1an:#569-tamer1an-fix-requestAllPages
Aug 25, 2019
Merged

j-rewerts merged 3 commits into
github-tools:masterfrom
tamer1an:#569-tamer1an-fix-requestAllPages

Conversation

@tamer1an

Copy link
Copy Markdown
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
Comment thread lib/Requestable.js
log(`getting next page: ${nextUrl}`);
return this._requestAllPages(nextUrl, options, cb, results);
if(nextUrl) {
options.page = parseInt(

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread lib/Requestable.js Outdated
return this._requestAllPages(nextUrl, options, cb, results);
if(nextUrl) {
options.page = parseInt(
nextUrl.match(/(&page=[0-9]*)/g)

Copy link
Copy Markdown
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
Copy Markdown
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

tamer1an commented Aug 26, 2019 •

Copy link
Copy Markdown
Contributor Author

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