Skip to content
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.

Sign up for GitHub

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

Don't double-paginate GitHub responses #506

Merged
merged 1 commit into from
Dec 10, 2014
Merged

Don't double-paginate GitHub responses #506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 6 additions & 29 deletions lib/github_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,10 @@ def remove_hook(full_github_name, hook_id)
end

def pull_request_comments(full_repo_name, pull_request_number)
paginate do |page|
client.pull_request_comments(
full_repo_name,
pull_request_number,
page: page
)
end
repo_path = Octokit::Repository.path full_repo_name

# client.pull_request_comments does not do auto-pagination.
client.paginate "#{repo_path}/pulls/#{pull_request_number}/comments"
Copy link
Member

Choose a reason for hiding this comment

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

I've actually added paginate to Octokit for pull_request_comments. But we haven't updated to that version of Octokit yet. Thoughts on doing that as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you maybe add pagination to pull_request**s**_comments? Octokit master doesn't paginate comments on a single pull request.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, duh! I bet I totally meant to add it to this one, but did the other one. 😝 octokit/octokit.rb#526

Copy link
Member

Choose a reason for hiding this comment

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

Hah: octokit/octokit.rb#543 Might not get a release gem right away though.

end

def pull_request_files(full_repo_name, number)
Expand Down Expand Up @@ -173,37 +170,17 @@ def create_team(name, repo)
end

def user_repos
repos = paginate { |page| client.repos(nil, page: page) }
authorized_repos(repos)
authorized_repos(client.repos)
end

def org_repos
repos = orgs.flat_map do |org|
paginate { |page| client.org_repos(org[:login], page: page) }
client.org_repos(org[:login])
end

authorized_repos(repos)
end

def paginate
page = 1
results = []
all_pages_fetched = false

until all_pages_fetched do
page_results = yield(page)

if page_results.empty?
all_pages_fetched = true
else
results += page_results
page += 1
end
end

results
end

def orgs
client.orgs
end
Expand Down
44 changes: 31 additions & 13 deletions spec/support/helpers/github_api_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,31 +354,39 @@ def stub_orgs_request(token)
end

def stub_paginated_repo_requests(token)
repos_url = "https://api.github.com/user/repos"

stub_request(
:get,
"https://api.github.com/user/repos?page=1&per_page=100"
"#{repos_url}?per_page=100"
).with(
headers: { "Authorization" => "token #{token}" }
).to_return(
status: 200,
body: File.read('spec/support/fixtures/github_repos_response_for_jimtom.json'),
headers: { 'Content-Type' => 'application/json; charset=utf-8' }
headers: {
"Link" => %(<#{repos_url}?page=2&per_page=100>; rel="next"),
"Content-Type" => "application/json; charset=utf-8",
}
)

stub_request(
:get,
"https://api.github.com/user/repos?page=2&per_page=100"
"#{repos_url}?page=2&per_page=100"
).with(
headers: { "Authorization" => "token #{token}" }
).to_return(
status: 200,
body: File.read('spec/support/fixtures/github_repos_response_for_jimtom.json'),
headers: { 'Content-Type' => 'application/json; charset=utf-8' }
headers: {
"Link" => %(<#{repos_url}?page=3&per_page=100>; rel="next"),
"Content-Type" => "application/json; charset=utf-8",
}
)

stub_request(
:get,
"https://api.github.com/user/repos?page=3&per_page=100"
"#{repos_url}?page=3&per_page=100"
).with(
headers: { "Authorization" => "token #{token}" }
).to_return(
Expand All @@ -389,31 +397,39 @@ def stub_paginated_repo_requests(token)
end

def stub_paginated_org_repo_requests(token)
org_repos_url = "https://api.github.com/orgs/thoughtbot/repos"

stub_request(
:get,
"https://api.github.com/orgs/thoughtbot/repos?page=1&per_page=100"
"#{org_repos_url}?per_page=100"
).with(
headers: { "Authorization" => "token #{token}" }
).to_return(
status: 200,
body: File.read('spec/support/fixtures/github_repos_response_for_jimtom.json'),
headers: { 'Content-Type' => 'application/json; charset=utf-8' }
headers: {
"Link" => %(<#{org_repos_url}?page=2&per_page=100>; rel="next"),
"Content-Type" => "application/json; charset=utf-8",
}
)

stub_request(
:get,
"https://api.github.com/orgs/thoughtbot/repos?page=2&per_page=100"
"#{org_repos_url}?page=2&per_page=100"
).with(
headers: { "Authorization" => "token #{token}" }
).to_return(
status: 200,
body: File.read('spec/support/fixtures/github_repos_response_for_jimtom.json'),
headers: { 'Content-Type' => 'application/json; charset=utf-8' }
headers: {
"Link" => %(<#{org_repos_url}?page=3&per_page=100>; rel="next"),
"Content-Type" => "application/json; charset=utf-8",
}
)

stub_request(
:get,
"https://api.github.com/orgs/thoughtbot/repos?page=3&per_page=100"
"#{org_repos_url}?page=3&per_page=100"
).with(
headers: { "Authorization" => "token #{token}" }
).to_return(
Expand Down Expand Up @@ -444,10 +460,12 @@ def stub_pull_request_comments_request(full_repo_name, pull_request_number)
"#{pull_request_number}/comments"
headers = { "Content-Type" => "application/json; charset=utf-8" }

stub_request(:get, "#{url}?page=1").
stub_request(:get, "#{url}?per_page=100").
with(headers: { "Authorization" => "token #{hound_token}" }).
to_return(status: 200, body: comments_body, headers: headers)
stub_request(:get, "#{url}?page=2").
to_return(status: 200, body: comments_body, headers: headers.merge(
"Link" => %(<#{url}?page=2&per_page=100>; rel="next"),
))
stub_request(:get, "#{url}?page=2&per_page=100").
to_return(status: 200, body: "[]", headers: headers)
end

Expand Down