Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

Open
alaycock wants to merge1 commit intogithub-tools:master
base:master
Choose a base branch
Loading
fromalaycock:master
Open
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletionslib/GitHub.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -91,11 +91,11 @@ class GitHub {

/**
* Create a new Search wrapper
* @param {string} query - the querytosearchfor
* @param {Search.Params} searchParameters - the queryand othersearchparameters
Copy link
Author

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.

* @return {Search}
*/
search(query) {
return new Search(query, this.__auth, this.__apiBase);
search(searchParameters) {
return new Search(searchParameters, this.__auth, this.__apiBase);
}

/**
Expand Down
35 changes: 26 additions & 9 deletionslib/Requestable.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -235,13 +235,30 @@ class Requestable {
* @param {string} path - the path to request
* @param {Object} options - the query parameters to include
* @param {Requestable.callback} [cb] - the function to receive the data. The returned data will always be an array.
* @param {Object[]} results - the partial results. This argument is intended for internal use only.
* @return {Promise} - a promise which will resolve when all pages have been fetched
* @deprecated This will be folded into {@link Requestable#_request} in the 2.0 release.
*/
_requestAllPages(path, options, cb, results) {
results = results || [];
_requestAllPages(path, options, cb) {
let manualPagination = false;
if (typeof options.page !== 'undefined') {
manualPagination = true;
}

return this._requestAllPagesHelper(path, options, cb, [], manualPagination);
}

/**
* Perform the logic of fetching multiple pages
* @private
* @param {string} path - the path to request
* @param {Object} options - the query parameters to include
* @param {Requestable.callback} [cb] - the function to receive the data. The returned data will always be an array.
* @param {Object[]} results - the partial results. This argument is intended for internal use only.
* @param {boolean} manualPagination - the flag to decide if multiple pages should be fetched
* @return {Promise} - a promise which will resolve when all pages have been fetched
* @deprecated This will be folded into {@link Requestable#_request} in the 2.0 release.
*/
_requestAllPagesHelper(path, options, cb, results, manualPagination) {
return this._request('GET', path, options)
.then((response) => {
let thisGroup;
Expand All@@ -256,19 +273,19 @@ class Requestable {
results.push(...thisGroup);

const nextUrl = getNextPage(response.headers.link);
if(nextUrl) {
if(nextUrl && !manualPagination) {
if (!options) {
options = {};
}
options.page = parseInt(
Copy link
Author

@alaycockalaycockSep 6, 2019•
edited
Loading

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.

nextUrl.match(/([&\?]page=[0-9]*)/g)
.shift()
.split('=')
.pop()
nextUrl.match(/([&\?]page=[0-9]*)/g)
.shift()
.split('=')
.pop()
);
if (!(options && typeof options.page !== 'number')) {
log(`getting next page: ${nextUrl}`);
return this._requestAllPages(nextUrl, options, cb, results);
return this._requestAllPagesHelper(nextUrl, options, cb, results, false);
}
}

Expand Down
2 changes: 2 additions & 0 deletionslib/Search.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -32,6 +32,8 @@ class Search extends Requestable {
* @param {string} sort - the sort field, one of `stars`, `forks`, or `updated`.
* Default is [best match](https://developer.github.com/v3/search/#ranking-search-results)
* @param {string} order - the ordering, either `asc` or `desc`
* @param {number} page - the page number to fetch, for manual pagination
* @param {number} per_page - the number of results to fetch per page, for manual pagination
*/
/**
* Perform a search on the GitHub API
Expand Down
Loading
[8]ページ先頭
©2009-2025 Movatter.jp