-
-
Notifications
You must be signed in to change notification settings - Fork 451
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? |
Hi @vitalik , is this something you intend to get merged into django-ninja, or would it make more sense for me to publish a separate package for this feature? |
Cross linking #385 |
Just FYI - I've been using cursor pagination for 6 months now based on this PR. |
Thanks @bufke . FWIW I am planning to publish this functionality as a 3rd party package in the coming days or weeks. I'll ping this thread when it's available. |
I have published this capability as a supplementary package: https://pypi.org/project/django-ninja-cursor-pagination/ I'll close this issue for now, but the maintainers should feel free to let me know if they'd like to upstream this into django-ninja and I'd be happy to help. (FYI @bufke ) |
I'm reopening this in case the maintainers want to keep this record. Feel free to close it. |
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.