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

Extract Django-Filter related code #3371

Closed
carltongibson opened this issue Sep 8, 2015 · 18 comments
Closed

Extract Django-Filter related code #3371

carltongibson opened this issue Sep 8, 2015 · 18 comments
Assignees
Labels
Milestone

Comments

@carltongibson
Copy link
Collaborator

As per discussion on #3315, it would be nice to pull the Django Filter related code out of DRF.

I'm happy to pull this into Django Filter itself. There's already an issue there to add a more DRF friendly base FilterSet class. It makes sense to combine the work into one.

Moving the code is simple enough — it should require a no more than a change of import for end users. Other queries: test coverage boundaries; docs.

@carltongibson carltongibson self-assigned this Sep 8, 2015
@xordoquy
Copy link
Collaborator

xordoquy commented Sep 8, 2015

We'll also need to follow the deprecation path on DRF about this.

@carltongibson
Copy link
Collaborator Author

@xordoquy — Ah, yes. Good point.

@tomchristie
Copy link
Member

In the past for cases where we can simply say "'x' is now an external package, make sure to install it." we've gone ahead and moved out of core without any further work. We could either:

  • Just force it as an external package.
  • Keep eg. rest_framework.filters.DjangoFilterBackend as a synonym during the deprecation process. (but still force the external install, rather than keeping the code locally)
  • Keep the code local (duplicated) during the deprecation process.

@carltongibson
Copy link
Collaborator Author

Since DjangoFilterBackend relies on Django-Filter (already) there's no harm removing the code — leaving the alias, as you say.

@tomchristie
Copy link
Member

Don't forget to use @jpadilla's excellent cookiecutter template when you take this on. :)

http://www.django-rest-framework.org/topics/third-party-resources/#how-to-create-a-third-party-package

I've still got an open task to remove the potential 'crispy-forms' dependency for the "filters in the browsable API" not sure best way to tackle that.

@carltongibson
Copy link
Collaborator Author

... excellent cookiecutter template ...

Just for my sanity, I'm thinking of putting this straight into Django Filter.

from django_filters.rest_framework import FilterBackend

... or such

... the potential 'crispy-forms' dependency ...

Yeah. I saw that. I'm in two-minds there.

At what point do we say, "Here's the hook and a simple default implementation — anything else you need is up to you"? i.e. Why not keep the dependency there — for the sake of working on (cough) better things?

@tomchristie
Copy link
Member

Just for my sanity, I'm thinking of putting this straight into Django Filter.

Great plan!

Why not keep the dependency there — for the sake of working on (cough) better things?

Think I'm happy either way - with crispy or without.

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 8, 2015

I'm tempted by the 3rd option - Keep the code local (duplicated) during the deprecation process.
I really believe DRF will benefit having a strong deprecation policy en particular due to its popularity and a relatively fast release cycle.

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 8, 2015

@carltongibson BTW, the link to the issue in this description is broken (404 here).

@carltongibson
Copy link
Collaborator Author

Yeah. OK fine. (Although the API and behaviour would remain unchanged even if we move the code — and it's the API and behaviour, not the implementation, that's subject to the deprecation policy right?)

I will work first on the DF side of it (this week). I'll open a PR moving the Filter Backend into DF.

Once the target end is in place I'll come back and we can settle how to handle it the DRF end.

(@xordoquy Ta. Fixed.)

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 8, 2015

it's the API and behaviour, not the implementation, that's subject to the deprecation policy right?

That's correct, but you may change a lot by requiring an update to an extra dependency in particular the valid Django version and co.

@carltongibson
Copy link
Collaborator Author

Sure. Lets review when we come to it.

@carltongibson
Copy link
Collaborator Author

Quick update: This is on my radar for over the holiday period. If that's not quick enough, PRs on Django Filter welcome :-)

@carltongibson
Copy link
Collaborator Author

I really believe DRF will benefit having a strong deprecation policy en particular due to its popularity and a relatively fast release cycle.

I think I agree here. Everyone I speak to who's using Django is using DRF.

@carltongibson
Copy link
Collaborator Author

carltongibson commented Jun 1, 2016

@tomchristie If you want something Django-Filter related. You could take this on. I'm happy to have this and maintain it in a django_filters.rest_framework module of the main Django-Filter repo — I just haven't got there.

Equally, there's no urgency to move it, so no rush. (Some of the other things seem more exciting.)

@tomchristie
Copy link
Member

I think this would be valid, yup.
Probably not top of my list, at the moment, tho possible that it'd be worth doing for the 3.4.0 release.

@carltongibson
Copy link
Collaborator Author

Right. Only about a year late but, progress.

Off the back of excellent effort from @rpkilby, we'll have the migrated DjangoFilterBackend code in Django Filter via carltongibson/django-filter#476

This will go in this week. It'll be on PyPI shortly afterwards.

What will be missing there is decent docs: the PR has been sat for ages™ waiting and I'd rather merge it and add them after than let the updated PR get stale with little conflicts again. (There are only a couple more points until DF hits Inbox Zero, and it's even going to have a 1.0 at last; the docs is part of that, so I'm happy they'll actually happen, at some point 🙂 )

I'm pinging to restart discussion on a roadmap and tasks — including documentation — for dropping the code from DRF.

@tomchristie
Copy link
Member

Very nice 👍😎

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 a pull request may close this issue.

3 participants