Open mjtorn opened 11 years ago
mjtorn
commented
11 years ago 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
commented
11 years ago 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
commented
11 years ago 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
commented
11 years ago 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.
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.