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.

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

Add paginator helper #40

Closed
wants to merge 69 commits into from
Closed

Add paginator helper #40

wants to merge 69 commits into from

Conversation

tmaier
Copy link
Member

@tmaier tmaier commented Jun 17, 2014

Unfinished and untested. Just to get the ball rolling.
Comments and suggestions are highly welcome.

  • Allow to set some options
  • Checks if order attribute is an accepted range
  • Take into account when options[:end] > count
  • Get data from request header
  • Parse request header
  • Allow to set order
  • Add tests
  • must set start and end later --> pagination with block

How to use

In your Endpoint:

If you want to return uuids in the Range header

Content-Range: id 01234567-89ab-cdef-0123-456789abcdef..01234567-89ab-cdef-0123-456789abcdef/400; max=200

Use uuid_paginator helper

If numbering the items is enough

Content-Range: id 0..199/400; max=200
paginator = integer_paginator(User.count)
query =
  User.order(paginator[:order_by])
    .limit(paginator[:limit])
    .offset(paginator[:offset])

Heroku documentation on ranges: https://devcenter.heroku.com/articles/platform-api-reference#ranges

@tmaier
Copy link
Member Author

tmaier commented Jun 29, 2014

Almost done. General feedback would be really appreciated

@tmaier
Copy link
Member Author

tmaier commented Jun 29, 2014

http://sequel.jeremyevans.net/rdoc-plugins/classes/Sequel/Dataset/Pagination.html

How to start pagination at a certain uuid?

@tmaier tmaier changed the title [WIP] Add paginator helper Add paginator helper Jun 30, 2014
end

describe '#[]' do
describe 'allows to read #res with a convinience method' do
Copy link
Member

Choose a reason for hiding this comment

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

*convenience

@brandur
Copy link
Member

brandur commented Jun 30, 2014

Wow, this is awesome. Thanks a lot @tmaier! It's always super interesting to see how someone else approaches an implementation.

So first of all, mind adding a set of tests for non-UUIDs, just so we can double-check that everything works when a range besides id is used?

On that same vein, we should also allow the "acceptable ranges" value to be passed into the paginator, probably during construction (or is the idea that res should be set by the user directly or by []?).

I'm not sure I precisely understood the usage examples. If I get the will_paginate? implementation correctly, it seems that it only takes into account the total range:

      def will_paginate?
        count > res[:args][:max].to_i
      end

Don't you want to account for your current page as well to know whether there will be another?

Lastly, would you mind adding it into the resource scaffold so we could see what a front to end usage of the paginator might look like?

I really like how explicit setting the next range is. Really awesome stuff.

/cc @pedro For a third set of eyes on this one.


def run
yield(self) if block_given?

Copy link
Member

Choose a reason for hiding this comment

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

Whitespace.

@tmaier
Copy link
Member Author

tmaier commented Jul 1, 2014

Thank you @brandur for your first comments and suggestions.

I will add and fix the issues mentioned within the next few days.
I will update this comment accordingly.

  • Add tests for non-uuids
    ba334c0
    421c814
  • How to set :accepted_ranges?
    There are two ways:
paginator(User.count, accepted_ranges: [:id, :foo, :bar])

and

paginator(User.count) do |paginator|
  paginator[:accepted_ranges] = [:id, :foo, :bar]
end
  • Fix will_paginate? to account for your current page
def will_paginate?
  count > res[:args][:max].to_i
end

The pagination will always happen if the number of items to show (max) is lower than the count of all available items. The offset is not necessary for this.

I have 200 chicken and want to see 20 at once: I need to paginate
(The fox came over night and ate 190 chicken)
I have 10 chicken and want to see 20 at once: I don't have to / can't paginate

  • Add it to the resource scaffold
    eaa3de0

Todo for pliny-template

@tmaier
Copy link
Member Author

tmaier commented Jul 1, 2014

Note the in 421c814 updated regex does not match IP addresses.

the following test will fail:

      describe 'ip addresses' do
        let(:sort_by) { 'ipv4' }
        let(:first) { '192.0.2.235' }
        let(:last) { '198.18.0.0/15' }

        it 'returns Hash' do
          result =
            {
              sort_by: sort_by,
              first: first,
              last: last,
              args: { max: '1000', order: 'desc' }
            }
          assert_equal result, subject.request_options
        end
      end

I came up with this regex to match also IPv4 addresses

VALUE = /(?:[^\.\s;\/]+|(?:\d{1,3}\.){3}\d{1,3}(\/\d+)?)/

It's complex and an edge case. I left it out for now.

If you know a regexp which matches all characters, but no blank, no semicolon and not two dots (\.{2}) following each other, I would be happy to hear about it.

@brandur
Copy link
Member

brandur commented Jul 1, 2014

@tmaier The paginator implementation definitely works best for values without dots in them, but I wouldn't worry too much about that for now though. We can improve the matching regex at a later time if issues start coming up.

tmaier added a commit to tmaier/pliny-template that referenced this pull request Jul 1, 2014
tmaier added a commit to tmaier/pliny-template that referenced this pull request Jul 1, 2014
@tmaier
Copy link
Member Author

tmaier commented Jul 1, 2014

Introduced uuid_paginator. This should be enough for most of the cases

get do
  MultiJson.encode uuid_paginator(Article, args: { max: 10 }).map { |x| serialize(x) }
end

@tmaier
Copy link
Member Author

tmaier commented Jul 1, 2014

@brandur I would say I'm done for now.

As you have access to two implementations, could you compare them and maybe even do a benchmark test to see what and how to improve the paginator

@tmaier
Copy link
Member Author

tmaier commented Jul 2, 2014

There are now three paginators.

  • paginator a general purpose paginator which is the base for the following. --> returns whatever you want
  • uuid_paginator use this one if you want to paginate uuids --> returns a Sequel dataset
  • integer_paginator to paginate a list of items. --> returns a hash

@tmaier
Copy link
Member Author

tmaier commented Aug 1, 2014

I would like to move this forward. Could anyone review it, please.

I'm happy about comments and willing to change the implementation if necessary.

@brandur
Copy link
Member

brandur commented Aug 6, 2014

@tmaier Oh man, this is serious fail on my part, and I apologize for it.
I promise to get to this pretty soon.

On Fri, Aug 01, 2014 at 08:52:42AM -0700, Tobias L. Maier wrote:

I would like to move this forward. Could anyone review it, please.

I'm happy about comments and willing to change the implementation if necessary.


Reply to this email directly or view it on GitHub:
#40 (comment)

@msakrejda
Copy link
Contributor

@brandur ... you promised...

@brandur
Copy link
Member

brandur commented Apr 16, 2015

I'm a horrible person :/

@uhoh-itsmaciek Do you need one of these or were you just looking at open pulls?

@msakrejda
Copy link
Contributor

The latter. I was browsing PRs after opening #146 and thought this seemed neat. But we're all horrible people; I'm just giving you a hard time.

It would be cool to see this in though. Let me know if I can help.

@DyegoCosta
Copy link
Contributor

+1
I'd love to see this in

@tmaier
Copy link
Member Author

tmaier commented Jan 5, 2016

Closing this in favor for #234. Hope that pagination support will land soon :)

@tmaier tmaier closed this Jan 5, 2016
@gudmundur
Copy link
Member

@tmaier I've been waiting for any kind of pagination to land. Still waiting for some of the details to settle on the other PR before going ahead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants