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

Change the way page_count is counted #1230

Closed
wants to merge 2 commits into from
Closed

Change the way page_count is counted #1230

wants to merge 2 commits into from

Conversation

gaato
Copy link
Contributor

@gaato gaato commented Apr 5, 2022

Summary

Changed the way Pagination.page_count is counted.

The documentation says a zero-indexed value, but since this is a count, not an index, it is more appropriate to count it normally.

It is not correct that page_count is -1 when there are no pages (which is not possible with the current code).

Did the previous page_count represent the index of the last page (0-based)?

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, typehinting, examples, ...)

BobDotCom
BobDotCom previously approved these changes Apr 5, 2022
@BobDotCom BobDotCom requested a review from krittick April 5, 2022 17:56
Dorukyum
Dorukyum previously approved these changes Apr 5, 2022
@krittick
Copy link
Contributor

krittick commented Apr 5, 2022

page_count is intended to be zero-indexed and is primarily used internally as such. Any values displayed to users are already converted to a more readable one-indexed value, but we should still be referring to it internally as a zero-indexed field for simplicity.

There are also better ways to handle the situation where there are no pages and page_count shows as -1 instead of changing the entire attribute indexing.

@krittick
Copy link
Contributor

krittick commented Apr 5, 2022

Note that there is development in progress to move a lot of Paginator attributes to properties, and having different page_count and page_index properties is part of that work. It's not a bad idea itself, but shouldn't be done as a separate breaking PR without that context around it.

@Lulalaby Lulalaby dismissed stale reviews from Dorukyum and BobDotCom April 5, 2022 18:55

X

@Lulalaby Lulalaby closed this Apr 5, 2022
@krittick
Copy link
Contributor

krittick commented Apr 5, 2022

As an aside, the use case you mentioned where page_count shows as -1 when there are no pages has been addressed in the fixes included in #1227.

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.

5 participants