-
Notifications
You must be signed in to change notification settings - Fork 299
Issue 430: pagination enhancement #434
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
Conversation
I think we should try to avoid adding new settings which DRF doesn't provide but simply add an example how to overwrite the class in the documentation (as you actually already do in this PR).
See http://www.django-rest-framework.org/api-guide/pagination/#modifying-the-pagination-style
Question is then how we want to solve problem how to move to new query param names..
What about adding a new class JsonApiPageNumberPagination and the like with the proper parameter names?
We could then deprecate the other classes PageNumberPagination and LimitOffsetPagination. To solve deprecation user can the move to JsonApi pagination classes or overwrite it to set parameters as needed.
What do you think?
In terms of Travis problem as workaround: You can simply push to git again and travis should pick it up.
e.g.
git commit --amend -m "Change to a slight different commit message"
git push -f@sliverc I think the workflow changes that were implemented are preventing Travis from running. It almost looks like it won't submit the Travis job until after an approving review.
@n2ygk what workflow changes are you referring to? There shouldn't be anything that would block Travis from running on PR branches. Travis normally runs everything before a review even happens.
I'm not sure why your PR is not running. If it would help us debug, I can try to push up a dummy PR to see if CI will run my branch. That might helps us determine if the problem is for the whole project or something with your specific repo or branch.
@n2ygk
Changes look good.
In terms of deprecation this could be done by adding a warning to __init__ of both classes like:
warnings.warn('PageNumberPagination is deprecated. Use JsonApiPageNumberPagination instead or create custom pagination (link to documentation)', DeprecationWarning)
Maybe this way we don't have to document deprecation in the documentation or this is simply additional. What do you think?
@sliverc Do you mean linking to the usage.md in the git repo or at readthedocs.io? The latter won't be there until release... Also the former won't be at the right place until merged.
And then there's the fact that right now I am subclassing them. I should swap that around now.
@n2ygk Fair point you would need to derive classes the other way around. Maybe a link to where it is gonna be linked to once it is released on readthedocs.io what do you think?
Codecov Report
@@ Coverage Diff @@ ## master #434 +/- ## ========================================== + Coverage 91.84% 91.92% +0.07% ========================================== Files 57 57 Lines 2957 2984 +27 ========================================== + Hits 2716 2743 +27 Misses 241 241
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Also love to see such a in depth documentation.
Do you think we should mentioned the deprecation in the changelog though?
This way when someone is upgrading they know what to do to remove deprecation warning to be safe in the future when we remove the class altogether.
@sliverc Thanks. Yes, I'll update the changelog; I always forget that part. I also want to add some tests. I'm having trouble forcing the warning to show up anywhere.
@n2ygk
For tests you might want to look at caplog https://docs.pytest.org/en/latest/logging.html#caplog-fixture
Found what I was doing wrong with the deprecation warnings: https://gregbrown.co/code/django-deprecation-warnings
I guess I'm using something newer than available in Python-2.7
@sliverc ready to merge.
I ended up using https://docs.pytest.org/en/latest/warnings.html for testing the deprecation warning. One of my tests was failing only under py27 so after wasting an hour debugging I made the decision that the effort wasn't worth the reward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just two small comments and we are ready to merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a pytest feature called xfail. See https://docs.pytest.org/en/latest/skipping.html
So instead of commenting this out you can mark it as @pytest.mark.xfail and still leave you comment with TODO.
This is that CI doesn't fail even when test is not successful but it still counts towards coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. Got caught by flake8 again (#436 will address this if and when done)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Methods name should always be all snake cased and lowercase.
👍 LGTM now. Thx.
Fixes #430
Description of the Change
Adds configurable pagination query parameter names along with settings defaults for them.
Note that changing the defaults of
pageandpage_sizewould be a breaking change sothose are left as-is but documentation shows how to change them to
page[number]andpage[size]for example.Checklist
CHANGELOG.mdAUTHORS