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

Add support for page_size parameter in CursorPaginator class #5250

Merged
merged 1 commit into from Sep 25, 2017

Conversation

cypreess
Copy link
Contributor

@cypreess cypreess commented Jul 5, 2017

CursorPaginator receives identical support for page size query parameter as PageNumberPagination.
Solving #5249

@tomchristie
Copy link
Member

tomchristie commented Jul 7, 2017

Hiya! The code looks good, but I'm still in two minds about if we should accept extra behavior on the already-complex cursor pagination. Not sure how to best take a judgement on this right now.

@cypreess
Copy link
Contributor Author

cypreess commented Jul 10, 2017

@tomchristie I actually don't understand where is that complexity you mention. In pagination you have two concepts:

  1. logic for finding page starting position - this can be anything, an offset, a cursor to value used for ordering, etc.
  2. logic for saying how many elements to retrieve from start position - here just a single integer param of page_size

The only doubt I have here is about the role of offset_cutoff in whole story.

@joshmaker
Copy link

joshmaker commented Sep 21, 2017

I hope this can be merged in at some point, because we really like this feature at The Atlantic. Ignoring the extensive unit tests, it's only a little bit more code and makes the CursorPaginator API work more like the other built in paginators.

@rpkilby rpkilby added this to the 3.7.0 Release milestone Sep 21, 2017
@carltongibson
Copy link
Collaborator

carltongibson commented Sep 25, 2017

OK, let's have this.

I was going to say "Needs Docs" but CursorPagination is already documented as taking a page_size param... 🙃

Thanks for the effort @cypreess!

@carltongibson carltongibson merged commit 60b9e58 into encode:master Sep 25, 2017
1 check passed
carltongibson added a commit that referenced this pull request Sep 25, 2017
carltongibson added a commit that referenced this pull request Sep 26, 2017
carltongibson added a commit that referenced this pull request Sep 27, 2017
carltongibson added a commit that referenced this pull request Sep 28, 2017
carltongibson added a commit that referenced this pull request Oct 5, 2017
carltongibson added a commit that referenced this pull request Oct 5, 2017
carltongibson pushed a commit that referenced this pull request Oct 6, 2017
* Set version number for 3.7.0 release

* Rename release notes section

Moved issue links to top for easier access.
(Can move back later)

* Add release note for #5273

* Add release note for #5440

* Add release note for #5265

Strict JSON handling

* Add release note for #5250

* Add release notes for #5170

* Add release notes for #5443

* Add release notes for #5448

* Add release notes for #5452

* Add release not for #5342

* Add release notes for 5454

* Add release notes for #5058 & #5457

Remove Django 1.8 & 1.9 from README and setup.py

* Release notes for merged 3.6.5 milestone tickets

Tickets migrated to 3.7.0 milestone.

* Add release notes for #5469

* Add release notes from AM 2ndOct

* Add final changes to the release notes.

* Add date and milestone link

Move issue links back to bottom.

* Update translations from transifex

* Begin releae anouncement

* Add release note for #5482

* 3.7 release announcement & related docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants