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

Add empty value choice for ChoiceFilter #519

Merged
merged 5 commits into from
Nov 2, 2016

Conversation

rpkilby
Copy link
Collaborator

@rpkilby rpkilby commented Oct 11, 2016

Adds empty and null value handling for ChoiceFilter, which should fix #45 and resolve #261.

  • Adds FILTERS_EMPTY_CHOICE_LABEL, FILTERS_NULL_CHOICE_LABEL, and FILTERS_NULL_CHOICE_VALUE settings, which allow you to globally set the empty/null choice labels and the null choice value.
    • Empty choice value is hardcoded as there shouldn't need to be a reason to override it.
    • Null choice value defaults to 'null', which should be a JS-friendly magic value.
  • Updated ChoiceFilter, which now takes corresponding empty_label, null_label, and null_value arguments. This is based on ModelChoiceField.empty_label.
    • empty_label and null_label may be set to None to disable those choices.
    • empty_label defaults to ---------, which maintains consistency w/ Django. This can be overridden globally or per filter.
    • null_label defaults to None, which means users must opt into using it.

TODO:

  • Add docs
  • Add tests
    • override empty_label value
    • test empty_label when None
    • test w/ other widgets (ChecboxSelectMultiple) There isn't anything to test here?
    • test w/ various filters (OrderingFilter, AllValuesFilter)

@carltongibson
Copy link
Owner

carltongibson commented Oct 11, 2016

OK. Good.

This handles the ANY or empty case. If you could just give the example of adjusting choices and overriding filter to add a NONE — similar to the example in #261 — I'd call this closed I think.

def __init__(...):
    super(...)...

   choices = ('NONE', 'None') + self.choices

def filter(self, qs, value):
    if value == 'NONE':
        return qs.filter(type_id=None)
   else:
      return super(...).filter(qs, value)

...or such.

Make sense?

@rpkilby rpkilby force-pushed the empty-value-choices branch 3 times, most recently from a99b431 to d6ca39a Compare October 12, 2016 03:04
@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 12, 2016

@carltongibson - I've updated the PR. I'll add docs/test if this all looks good.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 21, 2016

Hi @carltongibson - the PR has been redone w/ docs and tests

@carltongibson carltongibson added this to the 1.0 milestone Nov 2, 2016
@carltongibson
Copy link
Owner

Conflict in docs/ref/settings.txt

@rpkilby
Copy link
Collaborator Author

rpkilby commented Nov 2, 2016

Rebased.

@carltongibson
Copy link
Owner

https://travis-ci.org/carltongibson/django-filter/jobs/172588858#L593-L595

  File "/home/travis/build/carltongibson/django-filter/tests/test_filters.py", line 372, in ChoiceFilterTests
    @override_settings(
NameError: name 'override_settings' is not defined

@rpkilby
Copy link
Collaborator Author

rpkilby commented Nov 2, 2016

Just noticed that :(

In the middle of another rebase.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Nov 2, 2016

Already - tests should be good now.

@carltongibson carltongibson merged commit e7e6cda into carltongibson:develop Nov 2, 2016
@rpkilby rpkilby deleted the empty-value-choices branch November 2, 2016 13:21
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

2 participants