Adds a third option for strict to raise a Validation Error #136

wolfe · 2014-02-14T14:59:33Z

I supplied this third option so that Django Rest Framework could return a 400 w/ an appropriate error message.

If this is to your liking, Alex, I'd be glad to add some unit tests before you merge.

Note the bizarre constants True, False, "RAISE" -- I did this to support backward compatibility. If you recommend a cleaner way to manage the strict options, let me know. Someone here recommended another boolean option, but then there'd be 4 possible values of the two booleans, only three of which make sense. I think this is more robust.

carltongibson · 2014-09-30T07:52:36Z

Hi @wolfe — Thanks for the pull request. Sorry it's sat so long.

First the tests are failing — they need to be fixed.

Next, can you pull the help_text change out into a separate Issue/PR so that it can be assessed on it's own. (It might well be a good change but it doesn't belong hidden in here.)

Finally — can you explain — perhaps with pseudo code why we want this? What does it look like from the DRF perspective? (That will form the basis of the documenting the change too.)

Thanks — Lets get this resolved!

carltongibson · 2014-11-25T20:52:10Z

Hi @wolfe — I'd like to get this in. Are you still happy to work on it?

A few points:

Tests — yes, please. We need to make sure nothing breaks.
I'm really keen to improve the documentation so something explaining the usage would be awesome.
I agree True, False, "RAISE" is bizarre. Using an enum comes to mind but:
- Python <3.4
- Deprecating True and False, mapping them to the new values, but not just breaking them without warning.

wolfe · 2014-11-25T21:09:26Z

Intentions... Yes, glad to help. But been just too, too busy. Thank you
so much for the extra nudge!

On Tue, Nov 25, 2014 at 4:52 PM, Carlton Gibson notifications@github.com
wrote:

Hi @wolfe https://github.com/wolfe — I'd like to get this in. Are you
still happy to work on it?

A few points:

Tests — yes, please. We need to make sure nothing breaks.

I'm really keen to improve the documentation so something
explaining the usage would be awesome.
3.

I agree True, False, "RAISE" is bizarre. Using an enum
https://docs.python.org/3/library/enum.html comes to mind but:

Python <3.4

Deprecating True and False, mapping them to the new values, but
not just breaking them without warning.

—
Reply to this email directly or view it on GitHub
#136 (comment).

carltongibson · 2014-11-26T08:06:03Z

@wolfe — no problem! I think this would be a good addition so I'll keep nudging. :-)

wolfe · 2014-11-26T13:26:59Z

@carltongibson I agree with the suggestions. As for the Enum option, I see two choices --- (1) have a less robust fallback (just a plain class) for Python < 3.4 as now OR (2) Add the source code, https://hg.python.org/cpython/file/3.4/Lib/enum.py, for enum into the repo, and use it as a fallback import for getting the Enum class. Preference?

In any event, I agree that the values True and False should get auto-converted with deprecation warning.

carltongibson · 2014-11-26T13:31:09Z

As nice as an enum would be, lets just go for a regular class — for all versions — for now.

I don't want to add a dependency just for this.
Branching code on imports... again — not just for this.
Perhaps a wistful comment/docstring about the Golden Age to Come :-)

True/False: as you say.

Excellent. Thanks for the effort!

carltongibson · 2014-11-26T13:36:51Z

... lets just go for a regular class ...

I should say, unless you're particularly keen of course. :-)

Looking at that source file — it's not going to play nicely with Python 2.6... — which I don't think we can drop just yet.

wolfe · 2014-11-26T13:39:47Z

Agreed.

Version 0.9.1 Release

Version 0.9.2 Release

mdentremont · 2015-04-23T18:01:33Z

@carltongibson, I've taken @wolfe's changes and reapplied it on top of your develop branch.

I also made some tweaks to get the tests passing.

Would you be able to take a peek and let me know if I missed anything?

Thanks!

carltongibson · 2015-04-23T18:20:32Z

@mdentremont — That looks good. I've been totally snowed under with work but am planning on putting a release together over (say) the next week. I'll review properly and we'll get this in.

Awesome. Thanks!

mdentremont · 2015-04-23T19:43:06Z

@carltongibson Thanks for the quick reply, sounds great!

Version 0.10.0

carltongibson · 2015-05-16T19:45:42Z

django_filters/filterset.py

@@ -266,7 +276,8 @@ def __new__(cls, name, bases, attrs):
 class BaseFilterSet(object):
    filter_overrides = {}
    order_by_field = ORDER_BY_FIELD
-    strict = True
+    # What to on on validation errors


carltongibson · 2015-05-16T19:48:58Z

@wolfe @mdentremont Guys this is great.

I just have one issue remaining: can we add just a little to the usage docs explaining the new option? (It needn't be long but the docs are my main concern and I don't want them to get less comprehensive).

- Do not change the default "This is an exlusion filter" help text - Update test_default_field to expect 'Filter' instead of '' to match new behavior

- Also fix a typo

mdentremont · 2015-06-23T17:34:13Z

@carltongibson I have addressed the typo and added the usage docs!

wolfe · 2015-06-23T18:12:55Z

Thank you, Matt, for taking this on!

carltongibson · 2015-06-23T18:14:17Z

Good work both of you! I'll review and merge. Thanks!

carltongibson · 2015-07-01T09:16:15Z

@wolfe, @mdentremont — Thanks for this gentlemen. Very good.

Adds a third option for strict to raise a Validation Error

adam-snyder · 2015-07-24T14:07:32Z

Thank you for adding this feature. How soon can we expect it in the next release? Keep up the good work.

carltongibson · 2015-07-24T14:10:51Z

I want to roll in #264 and then I'll look at a new release. (Until then you can use develop)

adam-snyder · 2015-07-24T14:14:04Z

Sounds good. I have to wait for it to be published to pypi before I can use it for our deployment. Thanks again.

carltongibson · 2015-07-24T14:16:00Z

@adam-snyder You know you can pip install ... straight from a GitHub branch right? https://pip.pypa.io/en/latest/reference/pip_install.html#git

restless · 2015-08-24T21:24:22Z

What's the purpose of displaying _("Filter") as the default help text? I've just upgraded to latest django-filter and suddenly all my filter fields started to display extra "Filter" text. I found that this comes from this pull request (aioTV@90d244b).

wolfe · 2015-08-25T06:32:08Z

I don't recall the specifics... 'twas a while ago. But I agree it should
have been a separate pull request. Of course, the main point of the pull
request was to pay attention to the argument help_tex, so you can set
help_text='' if you prefer.

I do prefer consistency --- if help_text is auto-injected for one kind of
filter (the exclusion filter) it should be for all. I vaguely recalled that
this resulted in the best experience when combined with Django REST
Framework which can potentially auto-generate lots of fields with
auto-generated help text.

But I appreciate that the change was backward incompatible, and so may have
been ill-advised. Perhaps settings like DJANGO_FILTER_FILTER_HELP_TEXT and
DJANGO_FILTER_EXCLUSION_HELP_TEXT are advised?

On Mon, Aug 24, 2015 at 11:24 PM, Jakub Wiśniowski <notifications@github.com

wrote:

What's the purpose of displaying _("Filter") as the default help text?
I've just upgraded to latest django-filter and suddenly all my filter
fields started to display extra "Filter" text. I found that this comes from
this pull request (aioTV/django-filter@90d244b
aioTV@90d244b).

—
Reply to this email directly or view it on GitHub
#136 (comment).

restless · 2015-08-25T07:18:14Z

Thanks for explanation wolfe. Probably it's more reasonable in case of DRF than in case of a regular filter. I'd say that:

I think that no value for a regular filter field is ok and doesn't break consistency. I'd compare it to a situation where a developer is encouraged to avoid commenting the obvious code. The exclusion filter is breaking the obviousness about how filter works so it has its "comment".
Extra settings would work for me too

My preference is to go with 1 unless you think that it's good to keep the "filter' for DRF. It's just my 2 cents, as I think that some action is required here to avoid forcing users to override all help_texts in all their filters.

ad-m · 2015-08-26T19:01:07Z

👍 for any change here. I want avoid write everywhere help_text="". I was also surprised by the change. I now have everywhere the same comment, the constant repetition.

…o-filter#136

carltongibson · 2015-08-27T07:59:20Z

OK. We've got competing use-cases here. I'll add a flag to FilterSet (and propagate that to Filter) to enable/disable auto-populating the help text. This will keep it nice and declarative.

I'll have the default be disable auto-populating — I think that's less surprising.

Unless someone jumps in with a PR, that'll not be instantly.

In the meantime, rather than downgrade, I suggest overriding __init__ on your FilterSet to loop through filters and set the extras["help_text"] key there.

rjschave · 2015-08-28T01:54:59Z

I too was surprised by this change. I ended up doing what Carlton suggested with something like this:

# specify empty string for help text to suppress on the filter form
for key in self.filters.iteritems():
    self.filters[key[0]].extra.update(
        {'help_text': ''}
    )

…ter#136

* master: migrations Follow PEP 0008 Remove help text for filter's form - see see carltongibson/django-filter#136 new fields introduced according to the latest experts algorithm. Note: migrations are not created yet. Conflicts: company/forms.py pola/logic.py

wolfe force-pushed the develop branch from 2bcb2db to 674d9ee Compare August 24, 2014 17:32

carltongibson mentioned this pull request Nov 25, 2014

Should raise when a filter class for a field cannot be found (and strict=True) #186

Closed

Carlton Gibson added 2 commits December 3, 2014 09:37

Merge pull request carltongibson#201 from alex/develop

2876348

Version 0.9.1 Release

Merge pull request carltongibson#213 from alex/develop

11ee90a

Version 0.9.2 Release

mdentremont force-pushed the develop branch from 674d9ee to e12ced1 Compare April 23, 2015 16:46

Merge pull request carltongibson#241 from alex/develop

17176bf

Version 0.10.0

carltongibson reviewed May 16, 2015
View reviewed changes

wolfe and others added 4 commits June 9, 2015 20:53

Add a third strict option which raises a validation error

f671a77

Allow help_text to be overridden in Filter constructor

90d244b

Get unit tests passing again

e16ee11

- Do not change the default "This is an exlusion filter" help text - Update test_default_field to expect 'Filter' instead of '' to match new behavior

Add usage docs for new strictness option

53c08f9

- Also fix a typo

mdentremont force-pushed the develop branch from 8379678 to 53c08f9 Compare June 23, 2015 17:33

carltongibson pushed a commit that referenced this pull request Jul 1, 2015

Merge pull request #136 from aioTV/develop

73a391a

Adds a third option for strict to raise a Validation Error

carltongibson merged commit 73a391a into carltongibson:develop Jul 1, 2015

carltongibson mentioned this pull request Jul 8, 2015

Adding "required" option to a filter does not give proper result encode/django-rest-framework#3124

Closed

ad-m added a commit to watchdogpolska/feder that referenced this pull request Aug 26, 2015

Downgrade django-filter to remove help_text - see carltongibson/djang…

78fa489

…o-filter#136

carltongibson mentioned this pull request Sep 3, 2015

Have help text NOT added by default. #292

Closed

mik-laj pushed a commit to KlubJagiellonski/pola-backend that referenced this pull request Oct 14, 2015

Remove help text for filter's form - see see carltongibson/django-fil…

aa96a72

…ter#136

ad-m mentioned this pull request Jan 4, 2016

Add settings for help_text in Filter #348

Closed

ad-m mentioned this pull request Sep 14, 2016

Add settings conf, add deprecation warnings #484

Merged

rpkilby mentioned this pull request Oct 12, 2017

Rework validation, add queryset filter method #788

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a third option for strict to raise a Validation Error #136

Adds a third option for strict to raise a Validation Error #136

wolfe commented Feb 14, 2014

carltongibson commented Sep 30, 2014

carltongibson commented Nov 25, 2014

wolfe commented Nov 25, 2014

carltongibson commented Nov 26, 2014

wolfe commented Nov 26, 2014

carltongibson commented Nov 26, 2014

carltongibson commented Nov 26, 2014

wolfe commented Nov 26, 2014

mdentremont commented Apr 23, 2015

carltongibson commented Apr 23, 2015

mdentremont commented Apr 23, 2015

carltongibson May 16, 2015

carltongibson commented May 16, 2015

mdentremont commented Jun 23, 2015

wolfe commented Jun 23, 2015

carltongibson commented Jun 23, 2015

carltongibson commented Jul 1, 2015

adam-snyder commented Jul 24, 2015

carltongibson commented Jul 24, 2015

adam-snyder commented Jul 24, 2015

carltongibson commented Jul 24, 2015

restless commented Aug 24, 2015

wolfe commented Aug 25, 2015

restless commented Aug 25, 2015

ad-m commented Aug 26, 2015

carltongibson commented Aug 27, 2015

rjschave commented Aug 28, 2015

Adds a third option for strict to raise a Validation Error #136

Adds a third option for strict to raise a Validation Error #136

Conversation

wolfe commented Feb 14, 2014

carltongibson commented Sep 30, 2014

carltongibson commented Nov 25, 2014

wolfe commented Nov 25, 2014

carltongibson commented Nov 26, 2014

wolfe commented Nov 26, 2014

carltongibson commented Nov 26, 2014

carltongibson commented Nov 26, 2014

wolfe commented Nov 26, 2014

mdentremont commented Apr 23, 2015

carltongibson commented Apr 23, 2015

mdentremont commented Apr 23, 2015

carltongibson May 16, 2015

Choose a reason for hiding this comment

carltongibson commented May 16, 2015

mdentremont commented Jun 23, 2015

wolfe commented Jun 23, 2015

carltongibson commented Jun 23, 2015

carltongibson commented Jul 1, 2015

adam-snyder commented Jul 24, 2015

carltongibson commented Jul 24, 2015

adam-snyder commented Jul 24, 2015

carltongibson commented Jul 24, 2015

restless commented Aug 24, 2015

wolfe commented Aug 25, 2015

restless commented Aug 25, 2015

ad-m commented Aug 26, 2015

carltongibson commented Aug 27, 2015

rjschave commented Aug 28, 2015