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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paginated collection #16

Merged
merged 9 commits into from Feb 28, 2016

Conversation

Projects
None yet
2 participants
@rossta
Contributor

rossta commented Feb 1, 2016

Opening this PR up for discussion. It's preferable in my opinion for API Ruby clients to embed knowledge of iterating through paged collections with the use of #each. A good example of this is the Twitter gem, which provides the ability for enumerable collections to enumerate over each cursor with recursive calls to each.

For the Groove API, we can leverage the pagination rel links.

This PR demonstrates the change to ResourceCollection#each. Some of the tasks are complete, other remain open pending opinions on this change.

  • Enumerate all items in a paginated collection via next rel links
  • Return an enumerator when no block is given
  • Concatenate the results of successive page calls to the current collection, effectively caching the results
  • Respect the per_page parameter when calling successive pages

This PR also fixes an issue with the rake task for Travis.

@rossta

This comment has been minimized.

Contributor

rossta commented Feb 13, 2016

I had a chance to make this PR "feature complete" to give resource collections the ability to enumerate their own paginated relations.

collection.each { |item| yield item }
rel = @rels[:next] or return self
coll = rel.get(@options.except(:page))

This comment has been minimized.

@Fodoj

Fodoj Feb 17, 2016

Owner

What coll stands for?

This comment has been minimized.

@rossta

rossta Feb 17, 2016

Contributor

Short for collection since it represents the next instance of a ResourceCollection in the paginated set. Do you prefer collection or perhaps other?

This comment has been minimized.

@Fodoj

Fodoj Feb 23, 2016

Owner

collection.collection looks strange to me :) but other is also pretty unclear..

This comment has been minimized.

@Fodoj

Fodoj Feb 23, 2016

Owner

resource_collection would be ok

it "merges data" do
resource = GrooveHQ::ResourceCollection.new(client, @page_1)
resource.each.to_a

This comment has been minimized.

@Fodoj

Fodoj Feb 17, 2016

Owner

I am not sure I like this "each.to_a" chain everywhere, would be more convenient just to call resource.size, resource.map etc. What do you think?

This comment has been minimized.

@rossta

rossta Feb 17, 2016

Contributor

Right, I could instead call #map first to load the rest of the paginated set:

it "merges data" do
  resource = GrooveHQ::ResourceCollection.new(client, @page_1)

  expect(resource.map(&:title)).to eql(["Ticket 1", "Ticket 2", "Ticket 3"])
  expect(resource.collection.size).to eql(3)
end

ResourceCollection does not currently respond to #size so I could delegate this call to #collection. However, then the question is which other Array methods to delegate? Should the ResourceCollection implement #method_missing similarly to Resource does for its @data instance variable and check first if #collection responds to the call before passing to super? In other words:

def method_missing(method_sym, *arguments, &block)
  if collection.respond_to?(method_sym)
    collection.send(method_sym)
  else
    super
  end
end
@rossta

This comment has been minimized.

Contributor

rossta commented Feb 28, 2016

Anything else I can do on this PR?

stub_request(:get, "http://api.groovehq.dev/v1/tickets?page=3").
with(:headers => {'Authorization'=>'Bearer phantogram'}).
to_return(:body => page_3.to_json, status: 200)

This comment has been minimized.

@Fodoj

Fodoj Feb 28, 2016

Owner

Let's use new hash syntax here and on line 92

This comment has been minimized.

@rossta

rossta Feb 28, 2016

Contributor

Updated in a few places.

resource = GrooveHQ::ResourceCollection.new(client, data)
expect(resource.each).to be_instance_of(Enumerator)
expect(resource.each.first.name).to eql "When I am small"

This comment has been minimized.

@Fodoj

Fodoj Feb 28, 2016

Owner

Would resource.first.name also work?

This comment has been minimized.

@Fodoj

Fodoj Feb 28, 2016

Owner

Would resource.first.name also work?

This comment has been minimized.

@rossta

rossta Feb 28, 2016

Contributor

Yes, good catch.

@Fodoj

This comment has been minimized.

Owner

Fodoj commented Feb 28, 2016

@rossta except last two comments it's good to merge. Sorry it takes so long for review, I was kind of vacation frequently in last weeks

@Fodoj

This comment has been minimized.

Owner

Fodoj commented Feb 28, 2016

Many thanks!

Fodoj added a commit that referenced this pull request Feb 28, 2016

@Fodoj Fodoj merged commit a74a500 into Fodoj:master Feb 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Fodoj

This comment has been minimized.

Owner

Fodoj commented Feb 28, 2016

@rossta could you please check one thing: if you run complete test suite locally with bundle exec rspec spec, does it fail on three tests you've added? It does on my machine with correct API token exported :(

@rossta

This comment has been minimized.

Contributor

rossta commented Feb 28, 2016

Yes, it looks like when an integration spec is run before the resource_collection_spec.rb, WebMock stays disabled. I can open a new PR with a fix.

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