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

Use an own paginator #646

Merged
merged 10 commits into from Jun 12, 2017
Merged

Use an own paginator #646

merged 10 commits into from Jun 12, 2017

Conversation

floemker
Copy link
Contributor

The current pagination creates links for every page, which does not work
anymore for many pages. Use instead an own paginator, which creates the
following pagination control:

< 1 ... 5 6 7 8 9 10 11 12 13 14 ... 10 >

I.e. prev and next links, a link to the first and to the last page, and links
to the current page plus by default four surrounding pages.

The current pagination creates links for every page, which does not work
anymore for many pages. Use instead an own paginator, which creates the
following pagination control: < 1 ... 5 6 7 8 9 10 11 12 13 14 ... 10 >
@codecov-io
Copy link

codecov-io commented Jun 10, 2017

Codecov Report

Merging #646 into master will increase coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #646      +/-   ##
==========================================
+ Coverage   68.84%   69.06%   +0.22%     
==========================================
  Files          88       89       +1     
  Lines        4439     4471      +32     
==========================================
+ Hits         3056     3088      +32     
  Misses       1383     1383
Impacted Files Coverage Δ
src/wiki/forms.py 63.09% <ø> (ø) ⬆️
src/wiki/plugins/attachments/views.py 59% <100%> (+0.37%) ⬆️
src/wiki/core/paginator.py 100% <100%> (ø)
src/wiki/plugins/images/views.py 47% <100%> (+0.92%) ⬆️
src/wiki/plugins/globalhistory/views.py 100% <100%> (ø) ⬆️
src/wiki/views/article.py 65.55% <100%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56d1004...cb6b519. Read the comment docs.

super(WikiPaginator, self).__init__(*args, **kwargs)

def _get_page(self, *args, **kwargs):
self.curPage = super(WikiPaginator, self)._get_page(*args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment contract is too implicit. It means that page_range is broken if _get_page hasn't been called. Isn't there a way to just call this method directly from page_range ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjaoming Calling _get_page() from page_range() unfortunately does not work.
Source from the django paginator (django/core/paginator.py):

class Paginator(object):
    def _get_page(self, *args, **kwargs):
        """
        Returns an instance of a single page. This hook can be used by
        subclasses to use an alternative to the standard :cls:`Page` object.
        """
        return Page(*args, **kwargs)

class Page(collections.Sequence):
    def __init__(self, object_list, number, paginator):
        self.object_list = object_list
        self.number = number
        self.paginator = paginator

I.e. the page number is required. And that is exactly what is needed here in the
first place. Based on that I would say that it is guaranteed that _get_page()
is called before page_range().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried reordering this a bit

  1. We don't overwrite a weak internal method (the single underscore denotes to avoid this)
  2. renamed curPage to something with underscores, camelCase isn't used in Python normally :)

It's in floemker#4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More efficient is always good (int instead of a Page object). Committed.

@benjaoming
Copy link
Member

Seems necessary and timely, great idea! Made a PR on your branch and added a review comment.

@benjaoming benjaoming merged commit 88765dd into django-wiki:master Jun 12, 2017
@floemker floemker deleted the paginator branch June 13, 2017 00:41
benjaoming added a commit that referenced this pull request Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants