Skip to content
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

Add lookup_expr to MultipleChoiceFilter #1054

Merged
merged 1 commit into from
Mar 8, 2019
Merged

Add lookup_expr to MultipleChoiceFilter #1054

merged 1 commit into from
Mar 8, 2019

Conversation

rixx
Copy link
Contributor

@rixx rixx commented Mar 7, 2019

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.

@rixx
Copy link
Contributor Author

rixx commented Mar 7, 2019

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 . imports now appear after .. imports, probably due to a change in isort itself.

@carltongibson
Copy link
Owner

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... 😝

@rpkilby
Copy link
Collaborator

rpkilby commented Mar 7, 2019

Is it a bug in isort, or did isort intentionally change its sorting logic?

@carltongibson
Copy link
Owner

I didn’t have time to look yet. (Hint, hint. 🙂)

@rixx
Copy link
Contributor Author

rixx commented Mar 8, 2019

Discussion happened in PyCQA/isort#417. The related PR says "The new default
sort order is from furthest to closest (most dots to least dots)", so isort default behaviour has changed. Reverting to previous behaviour can be done with the -rr/--reverse-relative flag, or the reverse_relative setting. This option is present in the most recent bugfix releases.

Options:

  • Start using reverse_relative
  • Adjust imports

Either can be done via this PR or not, as you prefer. Just let me know :)

Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Just the one comment...

@@ -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')])
Copy link
Owner

Choose a reason for hiding this comment

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

So, what do we think about this change, and similar? (See other comment.)

@@ -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:
Copy link
Owner

Choose a reason for hiding this comment

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

self.lookup_expr is 'exact' by default. It's always truthy

Do we want if name and self.lookup_expr != 'exact':...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@carltongibson
Copy link
Owner

carltongibson commented Mar 8, 2019

@rixx If you could rebase after 3027c9e, that would be grand. (Did that.)

@codecov-io
Copy link

codecov-io commented Mar 8, 2019

Codecov Report

Merging #1054 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1054      +/-   ##
==========================================
+ Coverage   98.34%   98.34%   +<.01%     
==========================================
  Files          15       15              
  Lines        1205     1208       +3     
==========================================
+ Hits         1185     1188       +3     
  Misses         20       20
Impacted Files Coverage Δ
django_filters/filters.py 99.7% <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 3027c9e...a248517. Read the comment docs.

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.
Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK, I made some edits. Let's go with this. Thanks @rixx. Welcome aboard! 🎉

@rixx
Copy link
Contributor Author

rixx commented Mar 8, 2019

Thank you for letting me resurrect a feature request this old, and taking care of the MR!

@carltongibson
Copy link
Owner

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.

@carltongibson carltongibson merged commit 9d921c7 into carltongibson:master Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants