Fix FilterSet proxy #4620
Fix FilterSet proxy #4620
Conversation
There was a problem hiding this comment.
Great work on resolving this! One minor proposed change as above - let us know if you think the proposal doesn't make sense in some way, but otherwise I think it's worth us making sure that we're able to give friendly error messages in the case the django-filter isn't installed.
| return super(FilterSetMetaclass, cls).__new__(cls, name, bases, attrs) | ||
|
|
||
| class FilterSet(six.with_metaclass(FilterSetMetaclass, DFFilterSet)): | ||
| pass |
There was a problem hiding this comment.
For user-friendlyness we should also have a branch that includes the case where django_filters isn't is installed so that FilterSet imports still succeed, but raise an assertion error letting the user know that they need to install django-filter.
There was a problem hiding this comment.
This could just be...
if django_filters:
...
else:
def FilterSet(*args, **kwargs):
assert False, 'django-filter must be installed to use the `FilterSet` class'
There was a problem hiding this comment.
Makes sense. I've added a commit for this. It's a bit complicated since FilterSet is designed to be subclassed, so getting a nice error message at the point of use needed another metaclass.
| if bases != (object,): | ||
| assert False, 'django-filter must be installed to use the `FilterSet` class' | ||
| super(FilterSetMetaclass, self).__init__(name, bases, attrs) | ||
| _BaseFilterSet = object |
There was a problem hiding this comment.
Wondering if it'd be marginally cleaner to have FilterSet declaration inside the if case, rather than after it.
Then the else case could just have a FilterSet stub that raises an assertion error, rather than having a FilterSet metaclass.
There was a problem hiding this comment.
The metaclass is required if we want the assert to fire when FilterSet is subclassed. That way the context of the error is at the actual point of use. Another alternative is a dummy class with an assert in __init__, but then the error won't happen until you hit the view.
|
Reckon we're good to go with this. Anyone else want to chime in? |
|
So for the coverage stuff. How do you feel about adding a new tox env |
I don't understand what that'd change / resolve. I think we just want to turn off or dial down the coverage tests at some point. It's useful to see the changes, but I don't really want it passing or failing a pull request. |
|
Great stuff, thanks for your work on this! |
Description
Fix FilterSet proxying to be aware of parent metaclasses.
Closes #4619.