Lucky::Paginator::BackendHelpers.paginate makes it difficult to use paginator derivatives

[BrucePerens]

I have a derivative class of Paginator because I wanted a small change in its behavior. Lucky::Paginator::BackendHelpers.paginate is hard-coded call Paginator.new, so its code must be duplicated for each derivative class.

Please add Paginator.paginate as a class function of Paginate and move the code from Lucky::Paginator::BackendHelpers.paginate into there. Change it to call self.new rather than hardcoding the class name. Change Lucky::Paginator::BackendHelpers.paginate to call Paginator.paginate. Then, anyone who wishes to derive from the Paginator class can call MyPaginator.paginate.

I guess I could monkey-patch, but that's ugly.

[BrucePerens]

This would also mean moving paginate_array and paginator_per_page into the Paginate class, but it's cleaner to have paginator_per_page in the class anyway, because overriding it then can happen by deriving the Paginator class rather than monkey-patching or deriving Lucky::Paginator::BackendHelpers.

[jwoertink]

What was the behavior change you were looking to do originally? Would it make more sense to just make that change instead? Also, I think this is a Lucky related issue more than Avram, right? I didn't want to transfer the issue if it's related to a query deal.

[BrucePerens]

You are right, please transfer the issue.

Regarding the change I want to make, my application stores all of its pages as static pages, which was pretty easy to make work with Lucky. You only talk to the dynamic part when you are changing data. So, I wanted the path to a paginated page to use a path parameter, like get "/test/?:page" rather than a query parameter. Path parameters work without modification on the action side, the code just looks for a parameter rather than specifically for a query parameter. I just have to modify path_to_page to use a path parameter.

[BrucePerens]

Also, paginate currently gets the path that it generates from context.request.resource which is a bit of a kludge, isn't it? To be symmetrical with the way the rest of Lucky works, it should be tied to the path of the action, shouldn't it?

[jwoertink]

Ah, I see what you're saying. Yeah, I guess that makes sense. We probably should just have that configurable. Something that says where the page value comes from (i.e. path, query_param, other...). Thanks for reporting.

[BrucePerens]

I ended up with this:

class MyPaginator < Lucky::Paginator
  def self.paginate(
    params,
    full_path : String,
    query : Avram::Queryable(T),
    per_page : Int32 = 25
  ) : Tuple(self, Avram::Queryable(T)) forall T
    pages = self.new \
      page: paginator_page(params),
      per_page: per_page,
      item_count: query.clone.reset_order.reset_limit.reset_offset.select_count,
      full_path: full_path

    updated_query = query.limit(pages.per_page).offset(pages.offset)
    {pages, updated_query}
  end

  def self.paginator_page(params) : Int32
    params.get?(:page).try(&.to_i) || 1
  end

  def path_to_page(page_number : Int) : String
    "#{full_path}#{page_number}" # This is the only line I really wanted to change.
  end
end