Add lookup_expr to MultipleChoiceFilter #1054
Conversation
|
All failing tests are isort fails. They refer to imports I haven't touched – would you like me to push the corrected version? The only change is that |
|
Hold off on that. This happened on Django the other day... we ended up fixing and then reverting as isort tried to make up it's mind... 😝 |
|
Is it a bug in isort, or did isort intentionally change its sorting logic? |
|
I didn’t have time to look yet. (Hint, hint. 🙂) |
|
Discussion happened in PyCQA/isort#417. The related PR says "The new default Options:
Either can be done via this PR or not, as you prefer. Just let me know :) |
tests/test_filters.py
Outdated
| @@ -399,7 +399,7 @@ def test_filtering(self): | |||
| f.filter(qs, ['value']) | |||
|
|
|||
| self.assertEqual(mockQclass.call_args_list, | |||
| [mock.call(), mock.call(somefield='value')]) | |||
| [mock.call(), mock.call(somefield__exact='value')]) | |||
There was a problem hiding this comment.
So, what do we think about this change, and similar? (See other comment.)
django_filters/filters.py
Outdated
| @@ -253,10 +253,13 @@ def filter(self, qs, value): | |||
| return qs.distinct() if self.distinct else qs | |||
|
|
|||
| def get_filter_predicate(self, v): | |||
| name = self.field_name | |||
| if name and self.lookup_expr: | |||
There was a problem hiding this comment.
self.lookup_expr is 'exact' by default. It's always truthy
Do we want if name and self.lookup_expr != 'exact':...?
There was a problem hiding this comment.
Ah, that was from before I discovered (by way of tests) that this filter didn't use __exact, hence the superflous conditional. I opted to keep the __exact since pretty much all other filters default to __exact, but you're right that we might as well leave it off, too.
Codecov Report
@@ Coverage Diff @@
## master #1054 +/- ##
==========================================
+ Coverage 98.34% 98.34% +<.01%
==========================================
Files 15 15
Lines 1205 1208 +3
==========================================
+ Hits 1185 1188 +3
Misses 20 20
Continue to review full report at Codecov.
|
This was requested in #49 (back when lookup_expr was called lookup_type). This is a minor change, with the added advantage of making ``MultipleChoiceFilter`` perform more like other Filter classes, down to the explicit ``__exact`` filter. As the documentation only mentions ``lookup_expr`` to be available on Filter classes in general, and doesn't mention ``MultipleChoiceFilter`` to be different, no documentation change is necessary.
There was a problem hiding this comment.
OK, I made some edits. Let's go with this. Thanks @rixx. Welcome aboard! 🎉
|
Thank you for letting me resurrect a feature request this old, and taking care of the MR! |
|
No problem. Thank you for fixing it! (That was back in the 100+ open issues, no new features days, so nice for it to come back up now.) Let’s merge this. |
This was requested in #49 (back when lookup_expr was called
lookup_type). This is a minor change, with the added advantage of making
MultipleChoiceFilterperform more like other Filter classes, down tothe explicit
__exactfilter.As the documentation only mentions
lookup_exprto be available onFilter classes in general, and doesn't mention
MultipleChoiceFilterto be different, no documentation change is necessary.