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

DRF FilterBackend & FilterSet #429

Closed
wants to merge 7 commits into from

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Jun 16, 2016

This is an attempt at #275.

Notes:

  • Made FILTER_DEFAULTS a FilterSet class attribute. This allows the DRF FilterSet to modify the defaults without relying on filter_overrides. Using filter_overrides would complicate the API, as subclasses would have to copy the changes in addition to making their own.
  • Made filter_overrides a meta option, providing a deprecation notice for the class attribute. This seems more correct? Behavior modifying options should exist in the class meta, not on the class itself (which should just be the declarative filters). Either way, can revert this and raise in a separate issue - I'd argue that strict and order_by_field should also be moved to meta.
  • django_filters.rest_framework does not have a conditional dependency on DRF (as rest_framework.filters had on django-filter). It doesn't seem necessary, as you won't be loading the rest_framework sub-package unless DRF is installed.
  • The crispy_forms conditional dependency was kept. I don't really have an opinion/preference, but it's a minimal amount of code so why not?
  • Added DRF to the CI matrix.
  • The FilterBackend and tests are almost entirely a carbon copy of what exists on the current DRF master branch.

Questions:

  • Should the crispy_forms conditional dependency be kept?
  • Should we stop providing support for python 3.2 and 3.3?
    • Most likely, it will continue working even if not officially tested
    • It's complicating the build matrix and increasing test times
    • Still, they're technically supported until 1.8 EOL's in 2018 :\

TODO:

  • Add DRF relevant docs.
  • Refactor.
  • Wait for 453, simplify DRF requirement in testing matrix.
  • Wait for 459, which contains the filter_overrides move

@rpkilby rpkilby force-pushed the rest_framework branch 4 times, most recently from 43ef98d to af01d78 Compare June 16, 2016 03:44
@carltongibson
Copy link
Owner

@rpkilby Good work! Give me a couple of days to look at this. Awesome

models.DurationField: {
'filter_class': DurationFilter
},
models.AutoField: {'filter_class': NumberFilter},

Choose a reason for hiding this comment

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

Is there any reason why the formatting was changed here?

Copy link
Collaborator Author

@rpkilby rpkilby Jun 16, 2016

Choose a reason for hiding this comment

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

Trying to improve readability. It's definitely preference, but the compacted version is easier for me to scan than the expanded version.

@rpkilby rpkilby force-pushed the rest_framework branch 2 times, most recently from de40bd8 to da2b291 Compare August 2, 2016 05:47
Ryan P Kilby added 6 commits August 2, 2016 01:51
- Make FILTER_DEFAULTS a class attr. This will allow the DRF FilterSet
  to easily override the defaults, without relying on filter_overrides.
- Move filter_overrides to the meta options, provide deprecation notice.
@carltongibson
Copy link
Owner

@rpkilby I've updated this in #476. Very to happy to take PRs against that if you're keen. Otherwise I will clean it up and merge as is, leaving docs to come.

Thanks for the effort!

@rpkilby
Copy link
Collaborator Author

rpkilby commented Aug 25, 2016

What's your timeline on merging #476 - are you trying to get that done this weekend?

@carltongibson
Copy link
Owner

@rpkilby Yes. I'd like to get it in — even if it needs a bit of follow-up work.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Aug 25, 2016

Sounds good - I'll try to take a look tomorrow night. If I don't have time, can always submit a PR later.

@carltongibson
Copy link
Owner

@rpkilby Awesome. If you have limited time, I'm happy to tweak this if you can work on #472 — that way we get both in!

@rpkilby
Copy link
Collaborator Author

rpkilby commented Aug 25, 2016

It's more like - I'll either open my laptop or I won't. Road trip in the morning, tubing in the afternoon, but otherwise my evening should be clear 😄

@carltongibson
Copy link
Owner

@rpkilby OK. Cool. (Remember it's Open Source and doesn't own you! :-) )

@rpkilby
Copy link
Collaborator Author

rpkilby commented Sep 4, 2016

Hey @carltongibson I reviewed the rebased version and decided to do some additional rebasing. I don't think I can submit a PR to the other branch because of the rebasing - do you want to reopen this PR instead?

Changes:

@rpkilby
Copy link
Collaborator Author

rpkilby commented Sep 4, 2016

docs added (preview here).

@carltongibson
Copy link
Owner

GitHub doesn't want to re-open this one. (Not sure why.)

Have pushed 285d7f7 as #481. I'll merge that as is.

Great work! Awesome!

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.

None yet

3 participants