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 cursor pagination implementation #836

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

zachmullen
Copy link

This is a direct port of DRF's CursorPagination to django-ninja. It is designed to be completely API- and behaviorally-compatible with the DRF version; even opaque cursor URLs generated with DRF should still work under this implementation, which was a design goal as the downstream project I initially created this on was switching from DRF to ninja.

If the ninja maintainers approve of the general concept, I can add tests and whatever else is required to get this merged.

@vitalik
Copy link
Owner

vitalik commented Aug 29, 2023

Hi @zachmullen

that's a good start, but would be nice to add some test coverage and fix failing tests on old python versions

@zachmullen zachmullen force-pushed the cursor-pagination branch 2 times, most recently from b981cdf to bb75a9e Compare August 29, 2023 14:19
ninja/pagination.py Outdated Show resolved Hide resolved
@zachmullen
Copy link
Author

I've got the tests passing and added some of my own, but mypy is unhappy, and the coverage isn't at 100% and I don't feel comfortable inline-disabling it. This is all the work I will be able to do for a while, so let me know if you want to take over this branch, or else I can release this paginator as its own third party package.

@raianul
Copy link

raianul commented Oct 13, 2023

Any idea when this will merge ?

@jamesrkiger
Copy link
Contributor

@vitalik I would like to help get this MR up to your standards so it can be merged. From some local testing the basic functionality seems to work fine. So it seems like the main issue will be getting more testing coverage. Before I proceed, however, I was wondering if you could tell me whether the current testing for cursor pagination in this branch is up to your standards. It adds endpoints to the demo_project and runs tests through there. Looking at other testing in the main repo, however, I got the impression you might want something a bit more abstracted. Very few tests for other parts of django-ninja use the demo_project. Should I build on the current cursor pagination testing here or try to overhaul it before building out more coverage? Any input would be appreciated.

@bufke
Copy link

bufke commented Jan 2, 2024

I noticed that the existing pagination works with both querysets and lists. See existing pagination. This one does not support lists. It looks like it would be have be more careful around using .count and perhaps avoid attempting to order lists.

@bufke bufke mentioned this pull request Mar 2, 2024
@vitalik
Copy link
Owner

vitalik commented Apr 30, 2024

Hi @zachmullen

sorry for delay, we just got async pagination support so this PR got a bit spoiled with conflicts

@zachmullen
Copy link
Author

Thanks for the update @vitalk . What needs to be done to get this merged?

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

6 participants