Issue 430: pagination enhancement #434

n2ygk · 2018-05-05T21:23:13Z

Fixes #430

Description of the Change

Adds configurable pagination query parameter names along with settings defaults for them.
Note that changing the defaults of page and page_size would be a breaking change so
those are left as-is but documentation shows how to change them to page[number] and
page[size] for example.

Checklist

PR only contains one change (considered splitting up PR)
unit-test added (was already present)
documentation updated
changelog entry added to CHANGELOG.md
author name in AUTHORS

sliverc · 2018-05-07T07:15:03Z

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

n2ygk · 2018-05-08T12:57:35Z

Sounds like a plan. I’ll take a look in the next couple of days. I had pushed another commit to try to wake up Travis....

…

On Mon, May 7, 2018 at 3:15 AM Oliver Sauder ***@***.***> wrote: 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 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#434 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJ5dxrCA69s6kwZwOyoxKBz8YmRHoO6ks5tv_R5gaJpZM4TzzDJ> .

This reverts commit 4507ab9.

n2ygk · 2018-05-14T21:20:12Z

@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.

mblayman · 2018-05-14T22:10:39Z

@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 · 2018-05-14T23:58:13Z

Yes please do. I did request a review this time. I don’t recall if I’ve done that before.

…

On Mon, May 14, 2018 at 6:10 PM Matt Layman ***@***.***> wrote: @n2ygk <https://github.com/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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#434 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJ5d40G-E3l0wwU4c-hb94z2KoB9CZeks5tygDhgaJpZM4TzzDJ> .

sliverc · 2018-05-15T10:55:19Z

@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?

n2ygk · 2018-05-15T13:20:30Z

@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.

sliverc · 2018-05-15T13:26:43Z

@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-io · 2018-05-15T18:22:52Z

Codecov Report

Merging #434 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
rest_framework_json_api/pagination.py	`94.33% <100%> (+2.03%)`	⬆️
example/tests/unit/test_pagination.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad3dd29...31fe58b. Read the comment docs.

sliverc

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.

n2ygk · 2018-05-16T13:15:45Z

@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.

sliverc · 2018-05-16T13:47:43Z

@n2ygk
For tests you might want to look at caplog https://docs.pytest.org/en/latest/logging.html#caplog-fixture

n2ygk · 2018-05-16T14:23:30Z

Found what I was doing wrong with the deprecation warnings: https://gregbrown.co/code/django-deprecation-warnings

n2ygk · 2018-05-16T17:46:38Z

I guess I'm using something newer than available in Python-2.7

n2ygk · 2018-05-16T18:22:20Z

@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.

sliverc

Looks good. Just two small comments and we are ready to merge this.

sliverc · 2018-05-17T11:17:49Z

example/tests/unit/test_pagination.py

+        assert 'LimitOffsetPagination' in str(record[0].message)
+
+
+# TODO: This test fails under py27 but it's not clear why so just leave it out for now.


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.

done. Got caught by flake8 again (#436 will address this if and when done)

sliverc · 2018-05-17T11:18:11Z

example/tests/unit/test_pagination.py

@@ -76,3 +77,22 @@ def test_valid_offset_limit(self):

        assert queryset == list(range(offset + 1, next_offset + 1))
        assert content == expected_content
+
+    def test_LimitOffset_deprecation(self):


Methods name should always be all snake cased and lowercase.

sliverc · 2018-05-17T13:20:49Z

👍 LGTM now. Thx.

n2ygk added 2 commits May 5, 2018 17:11

Add configurable pagination query parameter names.

4507ab9

update changelog

6589946

n2ygk added the enhancement label May 5, 2018

n2ygk requested a review from sliverc May 5, 2018 21:30

n2ygk mentioned this pull request May 6, 2018

Request for new maintainer #424

Closed

n2ygk added 2 commits May 6, 2018 13:38

touch up changelog

9403a07

Merge remote-tracking branch 'upstream/master' into issue-430

fcdfa90

n2ygk added 2 commits May 11, 2018 14:43

Revert "Add configurable pagination query parameter names."

0b136eb

This reverts commit 4507ab9.

add JsonApiPageNumberPagination and JsonApiLimitOffsetPagination

863feeb

remove deprecated pagination class documentation

2b437dc

n2ygk changed the title ~~Issue 430: pagination enhancement~~ WIP: Issue 430: pagination enhancement May 15, 2018

n2ygk added 2 commits May 15, 2018 14:09

make JsonApi* paginators the base class and add deprecation warnings

0439081

flake8

7c53996

sliverc reviewed May 16, 2018

View reviewed changes

changelog; don't forget super init

8569fd9

add tests for pagination deprecation warnings

ab7fb38

remove trivial test that fails for no apparent reason under py27

f5195a9

n2ygk changed the title ~~WIP: Issue 430: pagination enhancement~~ Issue 430: pagination enhancement May 16, 2018

sliverc requested changes May 17, 2018

View reviewed changes

n2ygk added 2 commits May 17, 2018 08:57

xfail py27 test_pagination

284a3d6

flake8 tests missing from tox (django-json-api#436)

31fe58b

sliverc approved these changes May 17, 2018

View reviewed changes

n2ygk merged commit 409fb65 into django-json-api:master May 17, 2018

n2ygk deleted the issue-430 branch May 17, 2018 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 430: pagination enhancement #434

Issue 430: pagination enhancement #434

n2ygk commented May 5, 2018

sliverc commented May 7, 2018

n2ygk commented May 8, 2018 via email

n2ygk commented May 14, 2018

mblayman commented May 14, 2018

n2ygk commented May 14, 2018 via email

sliverc commented May 15, 2018

n2ygk commented May 15, 2018 •

edited

sliverc commented May 15, 2018

codecov-io commented May 15, 2018 •

edited

sliverc left a comment

n2ygk commented May 16, 2018

sliverc commented May 16, 2018

n2ygk commented May 16, 2018

n2ygk commented May 16, 2018

n2ygk commented May 16, 2018

sliverc left a comment

sliverc May 17, 2018

n2ygk May 17, 2018

sliverc May 17, 2018

sliverc commented May 17, 2018

		assert 'LimitOffsetPagination' in str(record[0].message)


		# TODO: This test fails under py27 but it's not clear why so just leave it out for now.

Issue 430: pagination enhancement #434

Issue 430: pagination enhancement #434

Conversation

n2ygk commented May 5, 2018

Description of the Change

Checklist

sliverc commented May 7, 2018

n2ygk commented May 8, 2018 via email

n2ygk commented May 14, 2018

mblayman commented May 14, 2018

n2ygk commented May 14, 2018 via email

sliverc commented May 15, 2018

n2ygk commented May 15, 2018 • edited

sliverc commented May 15, 2018

codecov-io commented May 15, 2018 • edited

Codecov Report

sliverc left a comment

Choose a reason for hiding this comment

n2ygk commented May 16, 2018

sliverc commented May 16, 2018

n2ygk commented May 16, 2018

n2ygk commented May 16, 2018

n2ygk commented May 16, 2018

sliverc left a comment

Choose a reason for hiding this comment

sliverc May 17, 2018

Choose a reason for hiding this comment

n2ygk May 17, 2018

Choose a reason for hiding this comment

sliverc May 17, 2018

Choose a reason for hiding this comment

sliverc commented May 17, 2018

n2ygk commented May 15, 2018 •

edited

codecov-io commented May 15, 2018 •

edited