Add support for counting grouped collections

[sliminas]

This PR solves the undefined method 'to_i' for Hash exception for grouped AR collections (#50).

[ddnexus]

Thanks @sliminas. I need to understand it better: couldn't that be avoided by grouping after the pagination? Please, could you post a specific example and use case? Thanks.

[KevinColemanInc]

I also just ran into this problem. It looks like kaminari covers this situation interally.

https://github.com/kaminari/kaminari/blob/4ada38c91128e4a5b9533f00516883d181881d46/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb#L33

[KevinColemanInc]

@sliminas

Can you also update this for the extras as well? they override the pagy_get_vars method.

[ddnexus]

@KevinColemanInc @sliminas

To me this looks like quite inefficient: you get it counted WITH distinct so it has to split the total count, and then re-join it, not to mention that you have to check every time whether it is one thing or the other... which doesn't come for free.

To me counting the full collection with distinct just to have a plain integer looks a bit off.

I asked before and I ask it again because I didn't try it: couldn't that be avoided by groping AFTER the pagination (which would mean that you get a plain count, without splitting and re-joining, and you finally get what you want when you need it?

Thanks

[ddnexus]

Can you also update this for the extras as well? they override the pagy_get_vars method.

@KevinColemanInc there is only one extra overriding pagy_get_vars (extra items), but it uses the built-in overridden and aliased method anyway

[sliminas]

@KevinColemanInc My solution for this was directly taken from how kaminari handles that.
@ddnexus I'll try that and give you an update.

[sliminas]

@ddnexus The setup I have:

class ProductsController < ApplicationController
  include Pagy::Backend

  def index
    # .... build some other objects depending on the user input
    @products = ProductFilter.filter(dynamic_filter_and_search_variables_and_objects)
     # @products sometimes is a grouped ActiveRecord::Relation
    @pagy, @products = pagy(@products)
  end 

end

The grouped collection I get is returned from a ProductFilter class which knows how to filter the products according to the search tearm, some filter objects and other parameters. For paginating it before grouping I could not use the very convenient Pagy::Backend#pagy mehtod since it's outside of the controller scope.

So I would have to copy all the logic from Pagy::Backend to the ProductFilter but I'm not sure if I also want that filter to handle the pagination.

[KevinColemanInc]

Basically the same story for me too.

Having the filter/finder classes also have to calculate their own count would be annoying to say the least.

At least for now, in my app, I would rather take the performance hit than spend the engineering hours to update all of the filters/finder classes to create a query for the results and a query for the count, unless there is an easy way to dynamically create that query across all group by queries.

[ddnexus]

@sliminas Thanks for the example
@KevinColemanInc I didn't mean necessarily doing it manually...

BTW, talking about an easy way that doesn't need any PR. It would be enough just defining an alias in the initializer like:

class Hash; alias :to_i :count; end

That would avoid checking for all the count results. I don't see any practical downside, and in this context it even makes sense: calling to_i on a hash would return its count.

[KevinColemanInc]

That makes sense. Maybe just update the docs then?

[ddnexus]

@KevinColemanInc The problem I still have with my own suggestion is that a grouped collection should be a case covered out of the box, because the generic Pagy::Backend defaults on AR. However in order to do so we should add that line somewhere in the loaded core code, and that means that we would add an alias to a core ruby class.

That is practical and elegant, makes sense, and doesn't hurt anything, but it is something that I would like to weight a bit more. On the other hand leaving it up to the user (maybe adding it in the initializer_example.rb) would not cover all the AR cases by default.

I am currently exploring other possibilities. Let's see if we could find a better way... at worse we have already 2 working solutions.

[ddnexus]

The alternatives I tried were not so great. Adding a to_i alias to the Hash class was just a bit faster than the proposed PR, but I didn't find any clean way to implement it.

The original proposed PR has been merged: I just changed the condition using is_a?(Hash) instead of respond_to?(:count) because it is a bit faster.

@sliminas @KevinColemanInc Thanks for the PR and the feedback.

[KevinColemanInc]

@ddnexus

The PR only fixes it in the default case, not in the add-ins that monkey patch that method. Could we also get this applied to them as well?

[ddnexus]

What "add-ins" are you talking about?

[KevinColemanInc]

I meant "extras", I didn't realize that the "parent" method was being called via alias_method:

https://github.com/ddnexus/pagy/blob/master/lib/pagy/extras/items.rb#L16

nevermind

[ddnexus]

Ah, OK. I was confused because I already told you that a few days ago:

@KevinColemanInc there is only one extra overriding pagy_get_vars (extra items), but it uses the built-in overridden and aliased method anyway