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

Move filter options to Meta class #459

Merged
merged 4 commits into from
Aug 23, 2016

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Aug 15, 2016

Extracted the strict, filter_overrides, and order_by_field moves to Meta from the other PRs. This resolves #430.

@carltongibson carltongibson modified the milestone: 0.15 Aug 22, 2016
@rpkilby
Copy link
Collaborator Author

rpkilby commented Aug 23, 2016

@carltongibson - btw, this is also ready for review.

@carltongibson carltongibson merged commit 3ea3e4c into carltongibson:develop Aug 23, 2016
@rpkilby
Copy link
Collaborator Author

rpkilby commented Aug 23, 2016

Sweet. I will rebase the other PRs later tonight*

*probably

@carltongibson
Copy link
Owner

@rpkilby No stress. It's just #429 that I really want to get in. Then I'll push 0.15 and we can start burning the deprecations. 🔥

Thanks for all the input!

@rpkilby rpkilby deleted the move-filter-options branch August 24, 2016 00:36
@blueyed
Copy link
Contributor

blueyed commented Aug 24, 2016

This causes an error with rest_framework_gis, which tries to update the filter_overrides: https://github.com/djangonauts/django-rest-framework-gis/blob/7ec925a97a1eae45f57733a49ddde7ddec119458/rest_framework_gis/filters.py#L87

I've only skimmed this, but it looks like handling this was not taken from #429?
Is it planned to be done there, or was it lost?

@carltongibson
Copy link
Owner

Well #429 is incoming, but the update will need to change location it looks like.

Exactly what are you referring to by "this" and "it" in "handling this" and "Is it planned"? (again I'm not quite following you immediately)

@blueyed
Copy link
Contributor

blueyed commented Aug 24, 2016

Sorry, I've meant to ask if it would work after #429, but only issue a deprecation warning (according to https://github.com/carltongibson/django-filter/pull/429/files#diff-c82ea95d2a317d98860bf154f27d3e17R181). And since this could have been done in this PR (#459) already, I've thought that it might have slipped through.

but the update will need to change location it looks like.

But it should not throw an error, but only warn about the deprecation, right?

@carltongibson
Copy link
Owner

But it should not throw an error, but only warn about the deprecation, right?

It's reference semantics, so it should be fine yes. I think :-)

@rpkilby
Copy link
Collaborator Author

rpkilby commented Aug 25, 2016

This will only work if filter_overrides is defined on the actual subclass. eg,

class MyFilterSet(GeoFilterSet):
    filter_overrides = {}
    ...

If you omit the attribute, there is no filter_overrides dict to update as it's been moved to Meta, thus an AttributeError

Also, the correct thing for rest_framework_gis to do after #429 merges is to copy & update the class FILTER_DEFAULTS.

@blueyed
Copy link
Contributor

blueyed commented Aug 25, 2016

Thanks for your clarification, @rpkilby.
I guess the next release should be versioned 1.0 then instead? (because of this API breaking change).

@carltongibson
Copy link
Owner

The breakage is covered by the 0.x already. I want to make the 1.0 after removing all the deprecations.

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.

3 participants