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
base: master
Are you sure you want to change the base?
Conversation
Hi @zachmullen that's a good start, but would be nice to add some test coverage and fix failing tests on old python versions |
b981cdf
to
bb75a9e
Compare
bb75a9e
to
eb6b1f2
Compare
c34b8a1
to
4b99b8c
Compare
4b99b8c
to
e0aa06e
Compare
I've got the tests passing and added some of my own, but |
Any idea when this will merge ? |
@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 |
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. |
Hi @zachmullen sorry for delay, we just got async pagination support so this PR got a bit spoiled with conflicts |
Thanks for the update @vitalk . What needs to be done to get this merged? |
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.