-
Notifications
You must be signed in to change notification settings - Fork 299
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
codecov-io
commented
Aug 24, 2018
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
- 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.
@sliverc I think this is really ready for your review now.
There was a problem hiding this 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.
@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.
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)
@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.
There was a problem hiding this 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.
Uh oh!
There was an error while loading. Please reload this page.
Fixes #432
Description of the Change
Implements
django_filters.DjangoFilterBackenda Django ORM-style JSON:APIfilter[]implementation. See docs/usage.md for details.Checklist
CHANGELOG.mdAUTHORS