JSONAPIOrderingFilter #459
Conversation
Codecov Report
@@ Coverage Diff @@
## master #459 +/- ##
==========================================
+ Coverage 93.24% 93.33% +0.08%
==========================================
Files 54 56 +2
Lines 3123 3164 +41
==========================================
+ Hits 2912 2953 +41
Misses 211 211
Continue to review full report at Codecov.
|
example/fixtures/blogentry.yaml
Outdated
| @@ -0,0 +1,60 @@ | |||
| - model: example.blog | |||
There was a problem hiding this comment.
let's also do this in JSON as in drf_example
example/tests/test_backends.py
Outdated
| """ | ||
| test sort | ||
| """ | ||
| response = self.client.get(ENTRIES + '?sort=headline') |
There was a problem hiding this comment.
parameters can be passed on as data e.g. in this case data={'sort': 'headline'}. This way there is no need for string concats
example/tests/test_backends.py
Outdated
| response = self.client.get(ENTRIES + '?sort=headline') | ||
| self.assertEqual(response.status_code, 200, | ||
| msg=response.content.decode("utf-8")) | ||
| j = json.loads(response.content.decode("utf-8")) |
There was a problem hiding this comment.
to get json result from response you should be able to simply do result = response.json() I would also prefer not to use single letter variables as python is not strongly typed so the name should tell what it which makes the code more readable (e.g. in this example result instead j or similar)
The same counts to the tests below.
There was a problem hiding this comment.
I've seen cases where response.json() doesn't work. Taking a look if this is one of them.
There was a problem hiding this comment.
but this is not one of them.
example/tests/test_backends.py
Outdated
|
|
||
| from ..models import Blog, Entry | ||
|
|
||
| ENTRIES = "/nopage-entries" |
There was a problem hiding this comment.
we shouldn't build routes manually but instead use django reverse resolver
In this case this would be reverse('nopage-entry'). It will be then a full path of the testserver url.
This needs to be done in setUp or what I usually do in each single test as it is more implicit.
There was a problem hiding this comment.
django.urls.exceptions.NoReverseMatch: Reverse for 'nopage-entry' not found. 'nopage-entry' is not a valid view function or pattern name.
There was a problem hiding this comment.
maybe nopage-entry-list
- use json instead of yaml fixture to remove dependency on PyYAML - use reverse() - request.data dict instead of string concat to the url - response.json() instead of json.loads(response.content.decode(...)
|
I am bit confused that you merged this before I had the chance to look at the changes you did again. I think it is important that we only merge once changes are approved. Reasons for this:
In this case the latter is actually the case. Just when looking at the code I have the feeling that when we pass on a camel cased variable e.g. blogEntry in case It would be great if you could verify this (best in a test) and open up another PR. Also another thing I noticed is I think we should stick to the same naming of modules as Django REST Framework does meaning the module should be named |
|
Sorry, I thought you indicated the changes were sufficient. Won’t do it
again.
…On Wed, Aug 22, 2018 at 3:08 AM Oliver Sauder ***@***.***> wrote:
I am bit confused that you merged this before I had the chance to look at
the changes you did again.
I think it is important that we only merge once changes are approved.
Reasons for this:
1. Requested changes need to be reviewed again whether there are done
as expected.
2. sometimes because of complexity of code errors might only be found
at a second glance
In this case the latter is actually the case. Just when looking at the
code I have the feeling that when we pass on a camel cased variable e.g.
blogEntry in case blog_entry sort field exists no error would be throw
but the parameter be removed and no sorting will happen because parent
class with remove_invalid_fields is called.
It would be great if you could verify this (best in a test) and open up
another PR.
Also another thing I noticed is I think we should stick to the same naming
of modules as Django REST Framework does meaning the module should be named
filters.py instead of backends.py. This seems to be more consistent to me.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#459 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEJ5d-BKtmxQVtYqm90t7nzpvk2xJ6sBks5uTQNugaJpZM4WEx9f>
.
|
|
@sliverc wrote:
I override However, I think I see a fundamental problem with the information-losing reversibility of camel case, etc. Namely, if the model uses camel case rather than the "preferred" underscore for field names, the I'll take a look at more explicit testing and submit a PR shortly. |
|
Of course you were correct, the sort is broken. Fixing. |
Fixes #432
Description of the Change
Partially addresses #432 by adding
JSONAPIOrderingFilterbackend. Broken up into smaller PR's per @sliverc.Checklist
CHANGELOG.mdAUTHORS