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

allow setting a custom Django Paginator in pagination.PageNumberPagin… #3631

Merged
merged 1 commit into from
Nov 30, 2015

Conversation

syphar
Copy link
Contributor

@syphar syphar commented Nov 13, 2015

…ation

@xordoquy
Copy link
Collaborator

What's the motivation behind that change ? Why isn't the django validator enough ?

@syphar
Copy link
Contributor Author

syphar commented Nov 13, 2015

@xordoquy in our codebase we are using a different Django Paginator for some bigger tables (where the count(*) takes too much time, and an estimated page-count is enough.

This would help us setting our own paginator without having to copy/paste the paginate_queryset method

@tomchristie
Copy link
Member

Out of curiosity, what do you use for estimated page counts?
Also, is modifying get_paginated_response an alternative here, or is that not sufficient?

Follow up... I guess num_pages performs a count() - might be good if we could avoid that?

@syphar
Copy link
Contributor Author

syphar commented Nov 13, 2015

we are using the postgres statistics for this table. it has currently around 3 Mio records, and the rough value is exact enough.

we already have modified get_paginated_response (similar to the Header based example in the docs, which removes one call to Paginator.count.

But paginate_queryset calls Paginator.num_pages, which also calls Paginator.count.

(a count in these specific tables takes around 2-3 seconds)

@xordoquy
Copy link
Collaborator

Sounds sensible to me.
Would probably rename the django_paginator_class to simply paginator_class

@tomchristie
Copy link
Member

django_paginator_class seems right - differentiate from our pagination controls.
I'm not 100% convinced if we want to add public API around this or not, but on balance I guess probably okay as-is.

@tomchristie
Copy link
Member

But paginate_queryset calls Paginator.num_pages, which also calls Paginator.count.

Indeed. Making this easier to eliminate might be a good plan

@tomchristie
Copy link
Member

Milestoning to push this to the top for a final review.

tomchristie added a commit that referenced this pull request Nov 30, 2015
allow setting a custom Django Paginator class
@tomchristie tomchristie merged commit 832d632 into encode:master Nov 30, 2015
@tomchristie
Copy link
Member

Thanks @syphar!

@syphar syphar deleted the paginat branch December 1, 2015 06:40
@syphar
Copy link
Contributor Author

syphar commented Dec 1, 2015

thanks @tomchristie and @xordoquy :)

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

Successfully merging this pull request may close these issues.

3 participants