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 a "not equal" lookup for filters #452

Merged
merged 1 commit into from Feb 11, 2020
Merged

Conversation

daviddavis
Copy link
Contributor

fixes #5868
https://pulp.plan.io/issues/5868

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/en/3.0/nightly/contributing/pull-request-walkthrough.html

daviddavis pushed a commit to daviddavis/pulp_rpm that referenced this pull request Dec 11, 2019
@Field.register_lookup
class NotEqualLookup(Lookup):
# this is copied from https://docs.djangoproject.com/en/3.0/howto/custom-lookups/
lookup_name = 'ne'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feedback welcome here on what we want to name this. The django example used ne.

@daviddavis daviddavis force-pushed the issue5868 branch 2 times, most recently from 82831e1 to c08e642 Compare December 11, 2019 19:06
daviddavis pushed a commit to daviddavis/pulp_rpm that referenced this pull request Dec 11, 2019
@daviddavis daviddavis changed the title Add a ne lookup for filters Add a "not equal" lookup for filters Dec 11, 2019
Copy link
Contributor

@dralley dralley left a comment

Choose a reason for hiding this comment

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

ne seems like an acceptable name for me

bmbouter
bmbouter previously approved these changes Dec 13, 2019
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

Thank you for this. I can't see a better way to do it in DRF or django-filter, so let's use this. 👍

@daviddavis
Copy link
Contributor Author

daviddavis commented Dec 13, 2019

I'm looking at a package called django-rest-framework-filters since it has support for negating filters:

https://github.com/philipn/django-rest-framework-filters#automatic-filter-negationexclusion

It basically adds some extra DRF functionality to django-filter.

@daviddavis
Copy link
Contributor Author

I had a chance to look into django-rest-framework-filters and it seems to work pretty well (at least for searching on NOT conditions). Moreover, you can combine it with other lookups (e.g. GET /pulp/api/v3/repositories/file/file/name__contains!=RHEL).

The only downside is we have to use a prerelease version from September 2018 (1.0.0.dev0) and it looks like there is no timeline for a 1.0.0 release.

I'm torn and don't have a strong opinion on whether to just go with this PR or use drf-filters.

@bmbouter
Copy link
Member

I also don't have a strong opinion. If it's working and that project is already dedicated to that type of feature set I lean towards using django-rest-framework-filters. @dralley do you see a downside to using django-rest-framework-filters?

@bmbouter bmbouter dismissed their stale review December 16, 2019 21:28

I think we should go w/ the django-rest-framework-filters project.

@dralley
Copy link
Contributor

dralley commented Dec 17, 2019

I agree that we should use the 3rd party package with one caveat:

We should make sure it's compatible with the range filters we currently support on some fields

https://github.com/philipn/django-rest-framework-filters#multiwidget-is-incompatible
https://github.com/pulp/pulpcore/blob/master/pulpcore/app/viewsets/base.py#L26

(I think it is compatible, but the range bit stuck out as something to look into)

@daviddavis
Copy link
Contributor Author

I plan to merge this on Monday February 10th. django-rest-framework-filters uses ! for NE fields which is a nonstarter for us since field names with ! won't work with the bindings.

daviddavis pushed a commit to daviddavis/pulp_rpm that referenced this pull request Feb 6, 2020
@daviddavis daviddavis merged commit 11385ce into pulp:master Feb 11, 2020
daviddavis pushed a commit to pulp/pulp_rpm that referenced this pull request Feb 11, 2020
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.

None yet

3 participants