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

fixed extra_validators parameter #584

Merged
merged 2 commits into from
Apr 22, 2020
Merged

Conversation

azmeuk
Copy link
Member

@azmeuk azmeuk commented Apr 22, 2020

BaseForm.validate method could take a extra_validators parameter, but Form.validate could not.

If this is ok I would also like to backport it to branch 2.3.x

@azmeuk azmeuk added the enhancement New feature, or existing feature improvement label Apr 22, 2020
@azmeuk azmeuk force-pushed the extra-validators branch from 40034b4 to c437564 Compare April 22, 2020 14:29
@davidism davidism changed the base branch from master to 2.3.x April 22, 2020 15:25
@davidism
Copy link
Member

davidism commented Apr 22, 2020

Rebased this to 2.3.x, added a changelog.

I copy extra_validators if it was passed in. This is to avoid the case where a dict is defined separately and passed to the function. If validate were called multiple times in that case, validate_fieldname would get appended to the same list multiple times. BaseForm.validate and Field.validate don't modify the value, so they don't need to be changed.

document extra_validators
@davidism davidism mentioned this pull request Apr 22, 2020
@davidism davidism merged commit 957a3f3 into pallets-eco:2.3.x Apr 22, 2020
@azmeuk
Copy link
Member Author

azmeuk commented Apr 22, 2020

I am not sure that copy is needed here. I think just having None as the default argument prevents the mutable default argument trap.

>>> d = {}
>>> def foobar(d=None): 
...     _d = d or {} 
...     _d.setdefault("foo", []).append("bar") 
...     return _d
>>> foobar(d)
{'foo': ['bar']}
>>> foobar(d)
{'foo': ['bar']}

However that would have been the case with d={} instead of d=None.

>>> def foobar(d={}): 
...     d.setdefault("foo", []).append("bar") 
...     return d
>>> foobar(d)
{'foo': ['bar']}
>>> foobar(d)
{'foo': ['bar', 'bar']}

@azmeuk azmeuk deleted the extra-validators branch April 22, 2020 16:37
@davidism
Copy link
Member

Your first test code doesn't seem to match up, you show it producing a string value instead of a list.

@azmeuk
Copy link
Member Author

azmeuk commented Apr 22, 2020

:) bad copy paste, now the snippet is accurate

@davidism
Copy link
Member

Ah, it's failing because your d dict is empty. If you do d = {"existing": ["value"]} you'll see the issue.

@azmeuk
Copy link
Member Author

azmeuk commented Apr 22, 2020

oooK now I see :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, or existing feature improvement
Development

Successfully merging this pull request may close these issues.

2 participants