revert JSONAPI prefix to JsonApi for paginators. #469

n2ygk · 2018-08-31T00:00:05Z

Fixes #471

Description of the Change

Reverts JSONAPI prefix to JsonApi prefix on paginators as in release 2.5.0. Will replace without
the JsonApi prefix in release 3.0. Deprecation warnings added now.

Checklist

PR only contains one change (considered splitting up PR)
unit-test added
documentation updated (including a few missed updates for other stuff in README.rst)
changelog entry added to CHANGELOG.md
author name in AUTHORS

Not sure it's needed, but the docmentation says to do it.

- allow the example app to still run, just failing any JSONAPIDjangoFilter tests. - split the two filters into separate files for easier maintenance.

…used

- Had a mistake (unquoted '.') and missing '-' as an allowed character. Also '_' already in '\w' - Don't be so exhaustive in testing for invalid filters; let JSONAPIQueryValidationFilter (when available) deal with that.

See django-json-api#466 (comment)

Per discussion about naming, the idea is that it should be easy to updgrade from DRF to DJA by simply changing some imports, retaining the same DRF (or in this case, django-filter) class names that are extended by DJA. see django-json-api#467 (comment)

…for consistency (django-json-api#463)" This reverts commit e6290af.

- see django-json-api#467 (comment) - JSONAPIPageNumberPagination goes back to PageNumberPagination - JSONAPILimitOffsetPagination goas back to LimitOffsetPagination - requires new setting: `JSON_API_STANDARD_PAGINATION=True` to get `page[number]` and `page[size]` instead of `page` and `page_size`. This is a breaking change no matter which is the default value.

sliverc · 2018-08-31T07:11:27Z

Let's try to follow common practices concerning deprecation and versioning.

I haven't had the time to document this but I think as DJA we should follow Semver and implement a similar deprecation policy like Ember (which is most likely the most used JS framework with JSON API spec).

This way we have a simple migration guide for our users. We can simply say, upgrade to the latest minor version of a series (e.g. 2.6.0) and solve all deprecation warnings. In version 3.0.0 we simply remove all deprecation warnings and do nothing else (real changes follow then in a version 3.1.0 etc.).

What this mean for our pagination:

We keep the naming of JSONAPIPageNumberPagination till version 2.6.0
In version 3.1.0 (or we could think of doing it in 3.0.0 as well) we make JSONAPIPageNumberPagination deprecated and change logic of PageNumberPagination to JSONAPIPageNumberPagination is today

This way we do not have to introduce a new arbitrary setting and have a simple migration guide for our DJA users.

What do you think?

n2ygk · 2018-08-31T21:36:43Z

I was assuming we were following semver.

Since 2.5.0 released JsonApi as the prefix, I think that should be what remains rather than an intermediate prefix of JSONAPI, which has not made it to a release yet. This involves reverting cfe89ea from master I believe.

Restating what you said, In addition we should add deprecation warnings now (presumably for a forthcoming 2.6.0 release) that indicate that in 3.0 or 3.1:

PageNumberPagination will change the default from page and page_size to page[number] and page[size], effectively doing what JsonApiPageNumberPagination does now.
LimitOffsetPagination will change the default max_limit from None to 100. Ditto.
JsonApiPageNumberPagination will go away, replaced by PageNumberPagination.
JsonApiLimitOffsetPagination will go away, replaced by LimitOffsetPagination.

I can revert the JSON_API_STANDARD_PAGINATION and so on in this PR that do more than the above.

Do you agree?

sliverc · 2018-09-04T07:47:58Z

This sounds good. 👍

We've decided to stick with JsonApi (as found in release 2.5.0) as the paginator prefix for now. This reverts commit 00dcf52.

(as is in release 2.5.0). Will replace without the JsonApi prefix in release 3.0. Deprecation warnings added.

n2ygk · 2018-09-07T00:20:28Z

rest_framework_json_api/filters/django_filter.py

@@ -0,0 +1,123 @@
+import re


this was added here by mistake. removing in next commit.

n2ygk · 2018-09-07T00:22:32Z

@sliverc This is ready for your review. I especially look forward to your thoughts regarding making a "promise" for future deprecations in release 3.0.

example/settings/dev.py

example/settings/test.py

docs/usage.md

rest_framework_json_api/pagination.py

- for renaming paginators without JsonApi prefix - for changes to attribute defaults

n2ygk · 2018-09-08T14:31:23Z

@sliverc I think I've done the PendingDeprecationWarnings correctly using self.__dict__ and also tried to document how to prevent getting caught by the deprecations.

sliverc · 2018-09-10T12:38:18Z

Thanks. The deprecation warnings look good now.

I tried to put myself into the shoes of an user and have a feeling they won't understand why they have to implement custom classes. We would certainly not recommend for them to do it unless they use the old parameters page and page_number.

To simplify the upgrade procedure for an existing user I think we should at this point not deprecate JsonApiPageNumberPagination. This way the documentation can state to move to JsonApiPageNumberPagination. Only if someone wants to keep the old parameters do they need to implement their custom pagination.

In the next release we will then deprecate JsonApiPageNumberPagination and say to move to PageNumberPagination.

Much cleaner for a user to understand what is happening.

Also how to solve deprecation needs to be part of the CHANGELOG and not hidden in the documentation.

I haven't written any detailed review because I think there might be a bit too many back and forth. Is it OK if I push to this PR? Would be more efficient and then you can look whether you agree with the changes.

n2ygk · 2018-09-10T13:12:14Z

@sliverc: Yes. Please do.

…

On Mon, Sep 10, 2018 at 8:38 AM Oliver Sauder ***@***.***> wrote: Thanks. The deprecation warnings look good now. I tried to put myself into the shoes of an user and have a feeling they won't understand why they have to implement custom classes. We would certainly not recommend for them to do it unless they use the old parameters page and page_number. To simplify the upgrade procedure for an existing user I think we should at this point not deprecate JsonApiPageNumberPagination. This way the documentation can state to move to JsonApiPageNumberPagination. Only if someone wants to keep the old parameters do they need to implement their custom pagination. In the next release we will then deprecate JsonApiPageNumberPagination and say to move to PageNumberPagination. Much cleaner for a user to understand what is happening. Also how to solve deprecation needs to be part of the CHANGELOG and not hidden in the documentation. I haven't written any detailed review because I think there might be a bit too many back and forth. Is it OK if I push to this PR? Would be more efficient and then you can look whether you agree with the changes. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#469 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJ5d0CZGSP3BEw5xdsaSYB1ORP7DIJDks5uZl0_gaJpZM4WUZik> .

…ginators

sliverc · 2018-09-12T07:47:24Z

@n2ygk I have adjusted the code and clarified changelog how to migrate. Does it look ok to you?

Do you see anything else which we need to do before releasing 2.6.0? Otherwise would I suggest we do a release and just short after follow up with a 3.0.0 release where we remove all deprecation warnings and deprecate JsonApiPageNumberPagination and JsonApiLimitOffsetPagination + changing defaults of PageNumberPaginiation and LimitOffsetPagination.

P.S.
We should squash merge this PR to keep a clear history in master

n2ygk · 2018-09-12T18:00:16Z

@sliverc This looks good and ready for you to squash merge.

Before a 2.6.0 release, please let me submit two more PRs:

QueryParameterValidation filter backend which is a companion to the others. This just enforces that only JSON:API-defined query parameters are allowed.
Documentation/examples update to show use of rest_framework.filters.SearchFilter with api_settings.SEARCH_PARAM = filter[search]
That should fully round out all the query param-based stuff.

I will want to get to some new exception classes that properly format the errors object but that can wait.

sliverc · 2018-09-13T06:49:22Z

Thanks. Merged.

I would prefer that we don't wait for a release concerning point 1. This can be added as fully backwards compatible change at any time later one. Point 2 makes certainly sense to document before a release.

n2ygk added 15 commits August 23, 2018 17:10

initial integration of JSONAPIDjangoFilter

7b29f36

documentation, isort, flake8

dc5ca38

Forgot to add django_filters to installed_apps

6b0dc8c

Not sure it's needed, but the docmentation says to do it.

backwards compatibility for py27 + django-filter

d4fbf24

handle optional django-filter package

d86d217

- allow the example app to still run, just failing any JSONAPIDjangoFilter tests. - split the two filters into separate files for easier maintenance.

fix travis to match new TOXENVs due to django-filter

83c4cc0

fixed a typo

f5792c1

add a warning if django-filter is missing and JSONAPIDjangoFilter is …

cbc9d55

…used

improve filter_regex

4f2b75b

- Had a mistake (unquoted '.') and missing '-' as an allowed character. Also '_' already in '\w' - Don't be so exhaustive in testing for invalid filters; let JSONAPIQueryValidationFilter (when available) deal with that.

easy changes recommended by @sliverc review

6a8d7ae

resolve @sliverc review method of using optional django-filter.

db9e1f9

See django-json-api#466 (comment)

Revert "deprecated JsonApi paginators class prefix to JSONAPI prefix …

cfe89ea

…for consistency (django-json-api#463)" This reverts commit e6290af.

override JSON_API_STANDARD_PAGINATION=False for test suite for now

fb17d78

sliverc mentioned this pull request Aug 31, 2018

Introduce versioning and deprecation policy #470

Closed

n2ygk added 3 commits September 6, 2018 13:00

Merge branch 'master' into remove-JSONAPI-paginators

ee236ea

Revert "revert JSONAPI prefix from paginators"

6145d33

We've decided to stick with JsonApi (as found in release 2.5.0) as the paginator prefix for now. This reverts commit 00dcf52.

Reverts JSONAPI prefix to JsonApi prefix on paginators

b90d983

(as is in release 2.5.0). Will replace without the JsonApi prefix in release 3.0. Deprecation warnings added.

n2ygk changed the title ~~WIP: remove JSONAPI prefix for paginators~~ revert JSONAPI prefix to JsonApi for paginators. Sep 7, 2018

n2ygk commented Sep 7, 2018

View reviewed changes

inadvertenly added a junked file

5a8d67e

n2ygk requested a review from sliverc September 7, 2018 00:25

sliverc requested changes Sep 7, 2018

View reviewed changes

PendingDeprecationWarnings

7c8b044

- for renaming paginators without JsonApi prefix - for changes to attribute defaults

sliverc mentioned this pull request Sep 10, 2018

Move to keep a changelog format #474

Merged

Oliver Sauder added 2 commits September 12, 2018 09:02

Merge remote-tracking branch 'upstream/master' into remove-JSONAPI-pa…

929251f

…ginators

Clarify deprecation warning of pagination classes

1c810c0

sliverc force-pushed the remove-JSONAPI-paginators branch from a781c13 to 1c810c0 Compare September 12, 2018 07:41

sliverc approved these changes Sep 13, 2018

View reviewed changes

sliverc merged commit 7fa46c2 into django-json-api:master Sep 13, 2018

n2ygk deleted the remove-JSONAPI-paginators branch September 13, 2018 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert JSONAPI prefix to JsonApi for paginators. #469

revert JSONAPI prefix to JsonApi for paginators. #469

n2ygk commented Aug 31, 2018 •

edited

Loading

sliverc commented Aug 31, 2018

n2ygk commented Aug 31, 2018

sliverc commented Sep 4, 2018

n2ygk Sep 7, 2018

n2ygk commented Sep 7, 2018

n2ygk commented Sep 8, 2018

sliverc commented Sep 10, 2018

n2ygk commented Sep 10, 2018 via email •

edited

Loading

sliverc commented Sep 12, 2018

n2ygk commented Sep 12, 2018

sliverc commented Sep 13, 2018

revert JSONAPI prefix to JsonApi for paginators. #469

revert JSONAPI prefix to JsonApi for paginators. #469

Conversation

n2ygk commented Aug 31, 2018 • edited Loading

Description of the Change

Checklist

sliverc commented Aug 31, 2018

n2ygk commented Aug 31, 2018

sliverc commented Sep 4, 2018

n2ygk Sep 7, 2018

Choose a reason for hiding this comment

n2ygk commented Sep 7, 2018

n2ygk commented Sep 8, 2018

sliverc commented Sep 10, 2018

n2ygk commented Sep 10, 2018 via email • edited Loading

sliverc commented Sep 12, 2018

n2ygk commented Sep 12, 2018

sliverc commented Sep 13, 2018

n2ygk commented Aug 31, 2018 •

edited

Loading

n2ygk commented Sep 10, 2018 via email •

edited

Loading