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

Fix FilterSet proxy #4620

Merged
merged 3 commits into from Nov 1, 2016
Merged

Fix FilterSet proxy #4620

merged 3 commits into from Nov 1, 2016

Conversation

spearki
Copy link
Contributor

@spearki spearki commented Oct 24, 2016

Description

Fix FilterSet proxying to be aware of parent metaclasses.

Closes #4619.

Copy link
Member

@tomchristie tomchristie left a 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
Copy link
Member

@tomchristie tomchristie Oct 24, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member

@tomchristie tomchristie Oct 24, 2016

Choose a reason for hiding this comment

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

This could just be...

if django_filters:
    ...
else:
    def FilterSet(*args, **kwargs):
        assert False, 'django-filter must be installed to use the `FilterSet` class'

Copy link
Contributor Author

@spearki spearki Oct 25, 2016

Choose a reason for hiding this comment

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

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.

@tomchristie tomchristie added this to the 3.5.2 Release milestone Oct 24, 2016
if bases != (object,):
assert False, 'django-filter must be installed to use the `FilterSet` class'
super(FilterSetMetaclass, self).__init__(name, bases, attrs)
_BaseFilterSet = object
Copy link
Member

@tomchristie tomchristie Oct 25, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@spearki spearki Oct 26, 2016

Choose a reason for hiding this comment

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

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.

@tomchristie
Copy link
Member

tomchristie commented Oct 26, 2016

Reckon we're good to go with this. Anyone else want to chime in?

@spearki
Copy link
Contributor Author

spearki commented Oct 26, 2016

So for the coverage stuff. How do you feel about adding a new tox env py27-bare that skips the requirements-optional.txt install?

@tomchristie
Copy link
Member

tomchristie commented Nov 1, 2016

How do you feel about adding a new tox env py27-bare that skips the requirements-optional.txt install?

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.

@tomchristie tomchristie merged commit 98df932 into encode:master Nov 1, 2016
1 of 3 checks passed
@tomchristie
Copy link
Member

tomchristie commented Nov 1, 2016

Great stuff, thanks for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants