- Notifications
You must be signed in to change notification settings - Fork786
Add pagination to search endpoints#583
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| /** | ||
| * Create a new Search wrapper | ||
| *@param {string} query - the querytosearchfor | ||
| *@param {Search.Params} searchParameters - the queryand othersearchparameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure if jsdoc understands how to reach into other files (Search.js). Let me know if this will be a problem.
| if(!options){ | ||
| options={}; | ||
| } | ||
| options.page=parseInt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If someone can clarify this, I would appreciate it.
Why does the page number need to be manually extracted and appended to the options?
nextUrl is being extracted from the headers, wouldn't it have all the options it already needs? Then by callingthis._requestAllPages(nextUrl, options, cb, results); the options are being re-added to the url, which results in duplicate parameters in the path, eg:
/search/repositories?q=tetris+language%3Aassembly&sort=stars&order=desc&type=all&per_page=100&page=4&q=tetris+language:assembly&sort=stars&order=desc&type=all&per_page=100&page=4
Although this might just be a workaround for other endpoints, since I have't done exploration into how other endpoints paginate.
Uh oh!
There was an error while loading.Please reload this page.
Changes:
Github.searchfunction.pageoption is passed into_requestAllPages, it will only fetch that single page, instead of attempting to fetch all of the pages.resultsparameter from_requestAllPages, since that is only meant to be used internally anyway. It's trivial to re-introduce, so let me know if this makes sense to do, since this could break some backwards compatibility if folks are using an unsupported argument.rawHeaderinstead ofheader, since the tests were not paginating. This was introduced when the spec for fixture files changed from nock@7 to nock@8.Let me know if this is the right approach. I've only run tests for the search endpoints (due to lack of access to the testing repo), so I'm not sure if this is going to introduce errors elsewhere.
Closes#406
This could be a potential solution for#460, although introducing a
limitcould be an alternate solution.