Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

revert JSONAPI prefix to JsonApi for paginators.#469

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

n2ygk
Copy link
Contributor

@n2ygkn2ygk commentedAug 31, 2018
edited
Loading

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 toCHANGELOG.md
  • author name inAUTHORS

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.
- 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.
Per discussion about naming, the idea is that it should be easy to updgrade from DRF to DJAby simply changing some imports, retaining the same DRF (or in this case, django-filter) classnames that are extended by DJA.seedjango-json-api#467 (comment)
- seedjango-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
Copy link
Member

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 followSemver and implement a similar deprecation policy likeEmber (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 ofJSONAPIPageNumberPagination till version 2.6.0
  • In version 3.1.0 (or we could think of doing it in 3.0.0 as well) we makeJSONAPIPageNumberPagination deprecated and change logic ofPageNumberPagination toJSONAPIPageNumberPagination 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
Copy link
ContributorAuthor

I was assuming we were following semver.

Since 2.5.0 releasedJsonApi as the prefix, I think that should be what remains rather than an intermediate prefix ofJSONAPI, which has not made it to a release yet. This involves revertingcfe89ea 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 frompage andpage_size topage[number] andpage[size], effectively doing what JsonApiPageNumberPagination does now.
  • LimitOffsetPagination will change the defaultmax_limit fromNone to100. Ditto.
  • JsonApiPageNumberPagination will go away, replaced byPageNumberPagination.
  • JsonApiLimitOffsetPagination will go away, replaced byLimitOffsetPagination.

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

Do you agree?

@sliverc
Copy link
Member

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 commit00dcf52.
(as is in release 2.5.0).Will replace without the JsonApi prefix in release 3.0. Deprecation warnings added.
@n2ygkn2ygk changed the titleWIP: remove JSONAPI prefix for paginatorsrevert JSONAPI prefix to JsonApi for paginators.Sep 7, 2018
@@ -0,0 +1,123 @@
import re
Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@n2ygk
Copy link
ContributorAuthor

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

@n2ygkn2ygk requested a review fromslivercSeptember 7, 2018 00:25
- for renaming paginators without JsonApi prefix- for changes to attribute defaults
@n2ygk
Copy link
ContributorAuthor

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

@sliverc
Copy link
Member

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 parameterspage andpage_number.

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

In the next release we will then deprecateJsonApiPageNumberPagination and say to move toPageNumberPagination.

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
Copy link
ContributorAuthor

n2ygk commentedSep 10, 2018 via email
edited
Loading

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

@slivercslivercforce-pushed theremove-JSONAPI-paginators branch froma781c13 to1c810c0CompareSeptember 12, 2018 07:41
@sliverc
Copy link
Member

@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
Copy link
ContributorAuthor

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

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

  1. QueryParameterValidation filter backend which is a companion to the others. This just enforces that only JSON:API-defined query parameters are allowed.
  2. Documentation/examples update to show use ofrest_framework.filters.SearchFilter withapi_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.

@slivercsliverc merged commit7fa46c2 intodjango-json-api:masterSep 13, 2018
@sliverc
Copy link
Member

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 reacted with thumbs up emoji

@n2ygkn2ygk deleted the remove-JSONAPI-paginators branchSeptember 13, 2018 15:27
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@slivercslivercsliverc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

naming style: JSONAPI prefix or not?
2 participants
@n2ygk@sliverc

[8]ページ先頭

©2009-2025 Movatter.jp