support github api pagination #10

Merged
merged 3 commits into from Mar 29, 2013

Projects

None yet

2 participants

@jdsn
jdsn commented Mar 22, 2013

The github api paginates its ouput if it contains too many elements.
This patch adds support for pagination. It collects and accumulates the paginated results.
See: http://developer.github.com/v3/#pagination

Using the parameter "per_page" could save some requests and thus time, but only some resources support this parameter.

@jdsn
jdsn commented Mar 22, 2013

The travis build failure seem to be unrelated to the patch. The errors also occur without it.

@jwilger
Owner
jwilger commented Mar 22, 2013

I haven't touched this project in a while. Looks like there was an issue with some of the gem dependencies and more recent patches of Ruby 1.9.3 that was making it impossible to run the specs. I updated the mater branch on my repo, which should now be able to run (and pass) the specs.

After merging your changes on top of those fixes, I'm seeing two spec failures that are not failing on master:

1048]% rake                                                                                                                                         jwilger tyrion:~/projects/github-v3-api  (jdsn-pagination)
/Users/jwilger/.rbenv/versions/1.9.3-p327/bin/ruby -S rspec spec/github_v3_api_spec.rb spec/issue_spec.rb spec/issues_api_spec.rb spec/org_spec.rb spec/orgs_api_spec.rb spec/repo_spec.rb spec/repos_api_spec.rb spec/user_api_spec.rb spec/user_spec.rb
....FF.........................................

Failures:

  1) GitHubV3API#get does a get request to the specified path at the GitHub API server and adds the access token
     Failure/Error: api.get('/some/path')
     NoMethodError:
       undefined method `headers' for "{}":String
     # ./lib/github_v3_api.rb:79:in `get'
     # ./spec/github_v3_api_spec.rb:35:in `block (3 levels) in <top (required)>'

  2) GitHubV3API#get returns the result of parsing the result body as JSON
     Failure/Error: api.get('/something').should == [{"foo" => "bar"}]
     NoMethodError:
       undefined method `headers' for "[{\"foo\": \"bar\"}]":String
     # ./lib/github_v3_api.rb:79:in `get'
     # ./spec/github_v3_api_spec.rb:41:in `block (3 levels) in <top (required)>'

Finished in 0.03917 seconds
47 examples, 2 failures

Failed examples:

rspec ./spec/github_v3_api_spec.rb:30 # GitHubV3API#get does a get request to the specified path at the GitHub API server and adds the access token
rspec ./spec/github_v3_api_spec.rb:38 # GitHubV3API#get returns the result of parsing the result body as JSON
rake aborted!

Are these failing for you as well if you bring in the changes from my master branch?

@jdsn
jdsn commented Mar 22, 2013

Seems like the spec is only returning the json body and not a proper RestClient object.
I'll try to find a solution (did not run the specs locally yet).

@jdsn
jdsn commented Mar 22, 2013

FYI: Indeed, the object returned from RestClient.get is a String that responds to the methods body, code and headers. So the stubbing needs to be adapted.

@jdsn
jdsn commented Mar 22, 2013

A little hacky, I agree, but it lets the tests pass.
Sorry, I did not find out how to mock the RestClient in a way that replies with a different answer the second time and only when requesting repos.

Hope you accept the patch anyway.

@jdsn
jdsn commented Mar 25, 2013

Is the current solution acceptable for you?
Otherwise I'd propose to implement a check in the get method if the returned object responds to the headers method. This should pass the tests as well and not require to change the String class.

@jwilger
Owner
jwilger commented Mar 25, 2013

Not a big fan of adding methods to a core class that are only present in the testing environment. I'd suggest just stubbing those out directly in the specs.

Speaking of specs, now that I'm actually looking at the code and not just the failed build, I notice there aren't any for the behavior changes you added. I'd definitely like to see some test coverage on this aspect before I'll merge it. (Apologies for not noticing that from the start; I was juggling a lot of projects at the end of last week.)

@jwilger
Owner
jwilger commented Mar 25, 2013

Probably the easiest way to do this would be to replace the strings returned in the GitHubV3API#get specs with a mock object that responds appropriately to the interface you are now calling in the new code. This should serve both to keep the existing tests passing and will also give you a way to test the new behavior.

@jdsn
jdsn commented Mar 27, 2013

I agree, it was a little dirty. The last commit adds a proper tests for the pagination and adapts two existing tests. They successfully run for me locally.
Anyhow travis had a issue (timeout when fetching gems from remote). I hope the test gets rebuilt once they sorted this.

@jwilger jwilger merged commit bf519a8 into jwilger:master Mar 29, 2013

1 check passed

Details default The Travis build passed
@jwilger
Owner
jwilger commented Mar 29, 2013

This is now pushed out to rubygems.org in 0.4.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment