rpkilby / django-filter

A generic system for filtering Django QuerySets based on user selections
https://django-filter.readthedocs.org
Other
0 stars 1 forks source link

0.14 release thoughts #4

Closed rpkilby closed 8 years ago

rpkilby commented 8 years ago

Hi @carltongibson - pinging you via my fork to not cause noise on the main repo. Also considered emailing you, but I'm not sure if you'd think it's appropriate/annoying.

Anyway, was thinking about the quickest path to the next release. So far, not much has really changed since the 0.13 release. The only real 'addition' is 412, which is just a minor extensibility improvement. Everything else is either minor bug fixes, docs improvements, or other various cleanups.

Changelog:

Minor cleanup:

The quickest path to a 0.14 release would probably be to merge 440 (deprecate container methods) and maybe 423 (AllValuesMultipleFilter). There doesn't seem to be anything contentious about the PRs, and there are no outstanding tests/docs required (side note, did submit a PR to cw0102's fork for a test on the filter, but honestly it's pretty inconsequential if the test doesn't get merged). Honestly, this almost could have been a patch release.

382 (Filter.method), 429 (DRF integration), 437 (Form improvements), and #3 (ordering filter rework) represent some fairly substantial changes and deprecations. The PRs still have some docs/testing/rebasing work that remains to be done, and I imagine there will be a bit of back & forth on design decisions and other cleanup before those can be successfully merged.

Roadmap?

rpkilby commented 8 years ago

Whoops - I think I forgot to ping you @carltongibson. Not sure if editing a comment will result in a notification.

carltongibson commented 8 years ago

Thanks for these thoughts. No problem at all with you contacting me.

I like the outline here. I think it's time for a 1.0.

My main concern with merging the PRs thus far is on the docs side — just making sure it's kept up.

I will get to this in the next few days to get the ball rolling.

Thanks for all the input!

rpkilby commented 8 years ago

I will get to this in the next few days to get the ball rolling.

Great.

My main concern with merging the PRs thus far is on the docs side

For 0.14, is there any docs work to be done? AllValuesMultipleFilter has docs in the PR, and everything else seems relatively minor. The changelog should be sufficient?

For the 0.15/1.0 PRs, I'll write the docs for a feature once it's been reviewed/given the okay. eg, you weren't sure about the Filter.method PR. Wouldn't want to spend time writing docs for a feature if it's rejected.

rpkilby commented 8 years ago

Hey - I've updated 440 with docs on deprecations and migration notes for 1.0. You can see them here: https://rpkilby.github.io/django-filter/migration.html

Whenever it's convenient, it would be helpful for 440 to be merged in advance as the other PRs are going to have to be rebased off of it (merge conflicts in the deprecation tests, adding to the migration doc).

carltongibson commented 8 years ago

@rpkilby — Yep.

I still need to think through the API for the MethodFilter changes. I'm inclined to keep it mirroring DRF, and eliminate everything else there. (The action kwarg etc) But open to your thoughts.

Other than that: All good! 👍

carltongibson commented 8 years ago

https://github.com/carltongibson/django-filter/milestones

rpkilby commented 8 years ago

I still need to think through the API for the MethodFilter changes. I'm inclined to keep it mirroring DRF, and eliminate everything else there. (The action kwarg etc) But open to your thoughts.

I'm still very inclined towards the PR. DRF's SerializerMethodField is read-only and doesn't need to be concerned with validation. On the other hand, filtering does need to validate its inputs, and MethodFilter does not currently provide a graceful way to do this. The correct way to handle it currently is to create a new subclass for each field type you want to validate, but this is kind of annoying.

Personally prefer this:

class UserFilter(FilterSet):
    pk = filters.NumberFilter(method='filter_pk')
    username = filters.CharFilter(method='filter_username')
    status = filters.ChoiceFilter(method='filter_status', choices=STATUS_CHOICES)

    # filter methods
    ...

over this:

class NumberMethodFilter(MethodFilter):
    field_class = forms.NumberField

class CharMethodFilter(MethodFilter):
    field_class = forms.CharField

class ChoiceMethodFilter(MethodFilter):
    field_class = forms.ChoiceField

class UserFilter(FilterSet):
    pk = filters.NumberMethodFilter()
    username = filters.CharMethodFilter()
    status = filters.ChoiceMethodFilter(choices=STATUS_CHOICES)

    # filter methods
    ...
carltongibson commented 8 years ago

Yeah. That's exactly it. OK. Good. 👍

rpkilby commented 8 years ago

Hey @carltongibson - 437 has been rebased and now passes the test suite. Looking to the upcoming releases, do you want to try and get it into the 0.15 and/or 1.0 release?

carltongibson commented 8 years ago

Hey @rpkilby — I was going to roll 0.15 this weekend. 1.0 will be a couple of weeks (at least 😀) after that. Unless you're extra keen, I'd aim for 1.0.

rpkilby commented 8 years ago

I'm not too worried about getting it into 0.15 - I did submit a separate PR for the settings changes though, given that the existing settings will be deprecated.

rpkilby commented 8 years ago

I think this is all tidied up.