search

zyga / django-pagination

Fork of unmaintained django-pagination - a set of utilities for creating robust pagination tools throughout a django application.
http://launchpad.net/linaro-django-pagination
BSD 3-Clause "New" or "Revised" License
39 stars 31 forks source link

Hopefully last pull request :) #17

Closed mjtorn closed 11 years ago

mjtorn commented 11 years ago

I found a bug with returning context, so the master was broken for a while :(

Not sure why, but returning the context there caused infinite recursion. The code is now cleaner and avoids this issue.

I also fixed the test settings so it's possible to run setup.py test but the cases don't pass. This appears to be an expected situation, as they don't pass in the upstream version?

Thanks!

mjtorn commented 11 years ago

Hey sorry, I think it's best to revert. I got confused over the old code base, it appears you must return the context in this case and until the issue with recursion is solved, this is not a good state for the code. I'll update this PR asap!

mjtorn commented 11 years ago

Do you happen to have a working test for this? I can confirm the correct _numpages in PaginateNode. It may be because of something we do, but considering there was already confusion and a test that didn't execute, it would be nice to hear a second opinion :) Thanks!

mjtorn commented 11 years ago

Wrote a test case for us which looks for both certain rendered variables from django-paginator's context and all that, it works.

zyga commented 11 years ago

I don't have any tests for this sadly (I'm not using it anymore and the extent that I did use it was limited).

zyga commented 11 years ago

BTW: do you still want this to land?

mjtorn commented 11 years ago

Sure, it helps against the current master, the confused code is reverted, the last one is a slight cleanup and now it's at least possible for someone (probably not me, I'm afraid) to look at setup.py test.

I found one issue but that's present even in your 0550c50 so I have to see if it's our code or not. Not today, haven't been feeling well and it's getting late.

As it stands, this PR is good to go IMO.

mktums commented 10 years ago

I have 

django.template.context in has_key
RuntimeError: maximum recursion depth exceeded while calling a Python object

with this PR merged.

Whole exception looks like this download

zyga commented 10 years ago

I'm sorry about that.

I don't actively maintain this project anymore but I will gladly merge patches to issues that affect its users. Can you work on a fix for this?

mktums commented 10 years ago

@zyga I'll try, but can't be sure about my skills… Also i ping'd stylesen in IRC channel, maybe he can consult me with this issue… Will work on this right now.

mktums commented 10 years ago

Found the bug. Blame this commit: b5f7065624d3f212ef9e22911478011789cc32b0 Don't know how to explain or revert it, though… I'm too newbie on this :(

mjtorn commented 10 years ago

It would appear I'm guilty as charged. After 10 months it's hard to remember anything that's happened and this works for us... The commit itself looks inconspicuous enough.

Do you have a concrete test case I could run against git bisect to see what's up?

Thanks!

mktums commented 10 years ago

@mjtorn set PAGINATION_INVALID_PAGE_RAISES_404 to False, go to page with pagination, set page URI param to unexisting page number.

In normal way it just should not list any items, but show the page. With current master it will be RuntimeError.

Sorry for providing use-case so lately

mktums commented 10 years ago

Ok, sorry for unprofessional behaviour of mine, it seems I figured it out.

Django's render_to_string() method awaits second argument (new_context in our case) to be dictionary, not Context instance. So when it tries to render template with this "dictionary" - it puts new Context instance within original Context instance, and the magic happens…