carltongibson / django-filter

A generic system for filtering Django QuerySets based on user selections
https://django-filter.readthedocs.io/en/main/
Other
4.41k stars 766 forks source link

Enhancement: pass named arguments to the custom method filters #1591

Closed aqeelat closed 1 year ago

aqeelat commented 1 year ago

Currently, when creating a custom filter method, we have to include all 3 parameters like this def my_filter(a, b, c), but what I don't need param b? It would be much better if we can pass named arguments so that we can do def my_filter(a, c, **kwargs)

carltongibson commented 1 year ago

I presume you're talking about the Filter.method argument.

Here the required parameters are queryset, name, and value. It's not clear how any of them are optional, but, in any case, complicating the API here isn't worth the confusion. (It's really not much of a burden to have to declared the correct function interface.)

If you have something else in mind, please let me know, and perhaps give a full example. Thanks.

aqeelat commented 1 year ago

This is an example copied from our code:

status = django_filters.CharFilter(field_name="status", lookup_expr="iexact", method="filter_status")

def filter_status(self, queryset, name, value):
    filtered_queryset = None
    if value == "pending":
        filtered_queryset = queryset.filter(status="pending")
    else:
        filtered_queryset = queryset.filter(status="confirmed", order__current_status__in=["cancelled", "delivered"])

    return filtered_queryset.prefetch_related()

queryset and value are indeed needed, but as you can see, name is not. I guess name is helpful when we're designing the filter method to be reusable, but not always.

carltongibson commented 1 year ago

Yes, you've hardcoded the name (status) here.

aqeelat commented 1 year ago

@carltongibson which means it is not always needed. The idea is not to complicate the API. it is just to replace: https://github.com/carltongibson/django-filter/blob/main/django_filters/filters.py#L807 with

        return self.method(queryset=qs, name=self.f.field_name, value=value)

I understand that this is a breaking change but you could have it behind a config variable if you think it is risky.

carltongibson commented 1 year ago

Yes, I understand. My point is that this isn't worth the disruption. It's not a change I'm going to make. Sorry.

aqeelat commented 1 year ago

No worries man. I just thought I'd ask.

carltongibson commented 1 year ago

No problem. Always worth asking. Thanks.