Skip to content
This repository was archived by the owner on Jun 5, 2018. It is now read-only.

RFD: Reuse existing paginator? #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mjtorn
Copy link

@mjtorn mjtorn commented Jun 26, 2013

Issue #18 describes how pagination, especially paginator.num_pages breaks if
{% autopaginate %} is called twice in the same template.

I do not understand how the render method works, at least for now, but my troubles
seem to go away, at least for now, if I re-use the existing paginator the other time
around.

Most likely this is the intended behavior originally and not having it so was a bug,
but I can't say for sure.

Apparently there is no state magick involved, so this should be ok from a
thread-safety point of view.

Do you think think it's ok? I'd hate to break it for everyone else.

@mjtorn
Copy link
Author

mjtorn commented Jul 11, 2013

Have you guys by any chance had time to look into this? I found that it's not perfect, though, as we still hit the database twice if we call the autopaginate tag twice. It may also have some fringe cases if autopaginate is called twice with different parameters.

@zyga
Copy link
Owner

zyga commented Jul 11, 2013

Hey. Sorry, I was a bit busy. I'll have a look into this.

The main thing that prevents me from landing this easily is that I don't use django-pagination in my projects and the test suite that was inherited is not really useful. I rely on downstream consumers telling me when it's broken or not. Any help in that area is strongly appreciated.

@mjtorn
Copy link
Author

mjtorn commented Jul 11, 2013

I totally feel that. You know... I could try our app against your master and see how it behaves. Unfortunately if we have to balance between going deep into this code or solve with caching, caching is the pragmatic solution.

I'll do that probably around next week or so, and if my results are not worth anything, we can close this pull request without pulling anything in. I think using many paginators is a very uncommon use-case; one I wish we wouldn't have to do.

(Also grr Django for not sharing context between template blocks properly)

@zyga
Copy link
Owner

zyga commented Jul 11, 2013

I think I'd like to see someone come up with a small set of tests for the paginator. Without that it's hard to merge anything.

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

Successfully merging this pull request may close these issues.

2 participants