django-json-api/django-rest-framework-json-apiPublic

NotificationsYou must be signed in to change notification settings
Fork302
Star1.2k

django_filters.DjangoFilterBackend#466

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

Jump to bottom

Merged

sliverc merged 17 commits intodjango-json-api:masterfromn2ygk:JSONAPIDjangoFilter

Sep 6, 2018

Merged

django_filters.DjangoFilterBackend#466

sliverc merged 17 commits intodjango-json-api:masterfromn2ygk:JSONAPIDjangoFilter

Sep 6, 2018

Conversation

Copy link

Contributor

n2ygk commentedAug 24, 2018•
edited
Loading

Fixes#432

Description of the Change

Implementsdjango_filters.DjangoFilterBackend a Django ORM-style JSON:APIfilter[] implementation. See docs/usage.md for details.

Checklist

PR only contains one change (considered splitting up PR)
unit-test added
documentation updated
changelog entry added toCHANGELOG.md
author name inAUTHORS

n2ygk added5 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.

n2ygk requested a review fromsliverc

August 24, 2018 22:57

n2ygk added2 commits

August 24, 2018 19:00

fix travis to match new TOXENVs due to django-filter

83c4cc0

fixed a typo

f5792c1

Copy link

codecov-io commentedAug 24, 2018

Codecov Report

Merging#466 intomaster willdecrease coverage by0.05%.
The diff coverage is92.43%.

@@            Coverage Diff             @@##           master     #466      +/-   ##==========================================- Coverage   93.44%   93.38%   -0.06%==========================================  Files          56       58       +2       Lines        3217     3400     +183     ==========================================+ Hits         3006     3175     +169- Misses        211      225      +14

Impacted Files	Coverage Δ
rest_framework_json_api/filters/sort.py	`100% <ø> (ø)`
example/urls_test.py	`100% <100%> (ø)`	⬆️
example/tests/test_filters.py	`100% <100%> (ø)`	⬆️
rest_framework_json_api/filters/__init__.py	`100% <100%> (ø)`
example/settings/dev.py	`91.3% <60%> (-8.7%)`	⬇️
rest_framework_json_api/filters/filter.py	`78.72% <78.72%> (ø)`
example/views.py	`91.91% <92%> (-0.09%)`	⬇️

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatea320536...f5792c1. Read thecomment docs.

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

cbc9d55

…used

Copy link

codecov-io commentedAug 25, 2018•
edited
Loading

Codecov Report

Merging#466 intomaster willincrease coverage by0.33%.
The diff coverage is100%.

@@            Coverage Diff             @@##           master     #466      +/-   ##==========================================+ Coverage   93.44%   93.77%   +0.33%==========================================  Files          56       58       +2       Lines        3217     3388     +171     ==========================================+ Hits         3006     3177     +171  Misses        211      211

Impacted Files	Coverage Δ
example/settings/dev.py	`100% <ø> (ø)`	⬆️
rest_framework_json_api/filters/sort.py	`100% <ø> (ø)`
example/views.py	`93.75% <100%> (+1.75%)`	⬆️
example/urls_test.py	`100% <100%> (ø)`	⬆️
rest_framework_json_api/filters/django_filter.py	`100% <100%> (ø)`
rest_framework_json_api/filters/__init__.py	`100% <100%> (ø)`
example/tests/test_filters.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatea320536...51b9946. Read thecomment docs.

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.

Copy link

ContributorAuthor

n2ygk commentedAug 28, 2018

@sliverc I think this isreally ready for your review now.

sliverc requested changes

Aug 29, 2018

View reviewed changes

Copy link

Member

sliverc left a comment

Choose a reason for hiding this comment

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

Nice work 👍 This will be a great addition to DJA. See my comments for questions and todos. Thanks for working on this.

example/views.py OutdatedShow resolvedHide resolved

rest_framework_json_api/filters/filter.py OutdatedShow resolvedHide resolved

requirements-development.txtShow resolvedHide resolved

rest_framework_json_api/filters/filter.py OutdatedShow resolvedHide resolved

n2ygk added2 commits

August 29, 2018 16:20

easy changes recommended by@slivercreview

6a8d7ae

resolve@slivercreview method of using optional django-filter.

db9e1f9

Seedjango-json-api#466 (comment)

Copy link

ContributorAuthor

n2ygk commentedAug 29, 2018

@sliverc thanks for the review! I believe I've adequately resolved most items. Please feel free to unresolve if you disagree.

I have left one open item which has to do with naming style. I think we need to clarify that further.

n2ygk mentioned this pull request

Aug 30, 2018

proposal: DJA DefaultRouter#467

Closed

rename JSONAPIDjangoFilter to DjangoFilterBackend.

51b9946

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)

Copy link

ContributorAuthor