#5833: Custom FilterSpecs -------------------------------------+------------------------------------- Reporter: | Owner: jkocherhans Honza_Kral | Status: assigned Type: New | Component: contrib.admin feature | Severity: Normal Milestone: | Keywords: nfa-someday Version: SVN | list_filter filterspec nfa- Resolution: | changelist ep2008 Triage Stage: Accepted | Has patch: 1 Needs documentation: 0 | Needs tests: 0 Patch needs improvement: 1 | Easy pickings: 0 -------------------------------------+------------------------------------- Changes (by julien):
* needs_better_patch: 0 => 1 * easy: => 0 Comment: Thanks for reviewing the patch! Replying to [comment:121 fas]: > - get_query_set should have no request parameter. It is passed in the __init__, you can set self.request there to be available for all the methods of the filter. has_output, title, ... could all depend on request. It is arbitrary that only get_query_set gets passed the request, especially if it can be accessed with self.request. I would prefer passing the request as a parameter for the sake of being explicit. This would also be more consistent with the way things work in other public APIs in Django, for example in `ModelAdmin`. > - there is no reason why _choices() should have an underscore (as introduced in the latest patch). It is not a private method and intended to be overridden in inheriting filters. The two main reasons I've added an underscore is because I haven't come up with a clean and simple API for `FieldListFilter` yet and also to avoid the developer accidentally overriding it instead of the `get_choices()` method which is part of the suggested new public API. If we're to remove the underscore then I'd like at least to find a more distinct name for it. > - I think the whole used_params and query_parameter_name thing is too restricting. Who says I use only one parameter? > The better implementation is to provide get_value with the parameter key and a default value, like this: > > {{{ > def get_value(self, name, default): > return self.params.get(name, default) > }}} > > You would then usually do something like this: > > {{{ > class MyCustomFilter(FilterSpec): > ALL = 'all' > MY_VALUE1 = 'some-param-value' > MY_VALUE2 = 'some-other-param-value' > # ... > DEFAULT_VALUE = MY_VALUE1 > > def __init__(self, ...): > super(...)... > self.arg = 'my-parameter-key' > self.value = self.get_value(self.arg, self.DEFAULT_VALUE) > > def get_query_set(self, qs): > if self.value == self.ALL: > return .... > if self.value == self.MY_VALUE1: > return ... > > > }}} > > The get_query_set method should be called no matter what is in the parameters (as there should be no query_parameter_name in the first place). > > - When applying the get_query_set method, if it returns None, it should not be used. The problem of having to check if to call the method based on request parameters disappears, as do custom checks in the method. This all seems interesting. Could you elaborate on how the developer would specify the choices/options available in the filter? Your approach seems valid but it still feels quite complex. I have been trying to come up with a very simple and easy API to cover most use cases. I believe that in most use cases one would just need one parameter. This is why I originally called the class `SimpleFilterSpec` (and in a later patch `SimpleListFilter`). But Jacob didn't like the split between `SimpleListFilter` and `FieldListFilter` (see his comment in http://groups.google.com/group/django- developers/browse_thread/thread/ec7c4124e7317145). I tend to agree with Jacob in that this split didn't seem ideal, but I also tend to agree with you that the current implementation of `ListFilter` is a bit too restrictive. Currently my preference would be to reintroduce `SimpleListFilter` to offer this dead-simple (yet restrictive) approach, and to modify `ListFilterBase` to accommodate for a less restrictive API (and also let `FieldListFilter` use that API to validate that it works). I'll continue thinking this through, but please feel free to provide any other criticisms or insights. -- Ticket URL: <http://code.djangoproject.com/ticket/5833#comment:122> Django <http://code.djangoproject.com/> The Web framework for perfectionists with deadlines. -- You received this message because you are subscribed to the Google Groups "Django updates" group. To post to this group, send email to django-updates@googlegroups.com. To unsubscribe from this group, send email to django-updates+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-updates?hl=en.