Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Sep 6, 2018
Merged

django_filters.DjangoFilterBackend #466

merged 17 commits into from
Sep 6, 2018

Conversation

n2ygk
Copy link
Contributor

@n2ygk n2ygk commented Aug 24, 2018 •
edited
Loading

Fixes #432

Description of the Change

Implements django_filters.DjangoFilterBackend a Django ORM-style JSON:API filter[] 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 to CHANGELOG.md
  • author name in AUTHORS

n2ygk added 5 commits August 23, 2018 17:10
@n2ygk
initial integration of JSONAPIDjangoFilter
7b29f36
@n2ygk
documentation, isort, flake8
dc5ca38
@n2ygk
Forgot to add django_filters to installed_apps
6b0dc8c 
Not sure it's needed, but the docmentation says to do it.
@n2ygk
backwards compatibility for py27 + django-filter
d4fbf24
@n2ygk
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 n2ygk requested a review from sliverc August 24, 2018 22:57
@codecov-io
Copy link

Codecov Report

Merging #466 into master will decrease coverage by 0.05%.
The diff coverage is 92.43%.

Impacted file tree graph

@@            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 by Codecov. Last update a320536...f5792c1. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Aug 25, 2018 •
edited
Loading

Codecov Report

Merging #466 into master will increase coverage by 0.33%.
The diff coverage is 100%.

Impacted file tree graph

@@            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 by Codecov. Last update a320536...51b9946. Read the comment docs.

@n2ygk
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.
@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 28, 2018

@sliverc I think this is really ready for your review now.

sliverc
sliverc requested changes Aug 29, 2018
View reviewed changes
Copy link
Member

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

@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 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 n2ygk mentioned this pull request Aug 30, 2018
Closed
@n2ygk
rename JSONAPIDjangoFilter to DjangoFilterBackend.
51b9946 
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)
@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 30, 2018

@sliverc If you agree with #467 (comment) then I think this is ready for final review and merge.

If you disagree I can revert 51b9946 and we can work it out.

@n2ygk n2ygk mentioned this pull request Aug 31, 2018
Merged
5 tasks
@n2ygk n2ygk changed the title JSONAPIDjangoFilter filters.DjangoFilterBackend Aug 31, 2018
@n2ygk n2ygk mentioned this pull request Aug 31, 2018
Closed
5 tasks
sliverc
sliverc reviewed Sep 4, 2018
View reviewed changes
sliverc
sliverc requested changes Sep 4, 2018
View reviewed changes
Copy link
Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

This PR is shaping up nicely. Just see my comments for some hopefully last changes before merging.

@n2ygk n2ygk changed the title filters.DjangoFilterBackend django_filters.DjangoFilterBackend Sep 5, 2018
@sliverc sliverc merged commit 5de570c into django-json-api:master Sep 6, 2018
This was referenced Sep 18, 2018
Closed
Merged
@n2ygk n2ygk added this to the 2.6.0 milestone Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sliverc sliverc sliverc approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@n2ygk @codecov-io @sliverc