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

Adds a validator check when creating a field #298

Merged
merged 1 commit into from
Jun 12, 2018
Merged

Conversation

adrcad
Copy link
Contributor

@adrcad adrcad commented Sep 17, 2016

When doing *Field(*args, validators=[DummyValidator]), there is no check
on the callable received (DummyValidator). For builtin validators, if
this is not an instance, you get an incomprehensible error from
_run_validation_chain. This commit adds a check at field's
initialization time to ensure either a callable instance object or a
function has been given as a validator by raising a meaningful error
message.

When doing *Field(*args, validators=[DummyValidator]), there is no check
on the callable received (DummyValidator). For builtin validators, if
this is not an instance, you get an incomprehensible error from
_run_validation_chain. This commit adds a check at field's
initialization time to ensure either a callable instance object or a
function has been given as a validator by raising a meaningful error
message.
Copy link
Contributor Author

@adrcad adrcad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 this PR can save time when debugging in case of a typo (you only typed the name of the validator class without the parenthesis - this is what actually triggered this PR)

@@ -154,6 +157,14 @@ def __call__(self, **kwargs):
"""
return self.meta.render_field(self, kwargs)

@classmethod
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

classmethod decorator needed to be able to perform a check even when instantiating an unbounded Field (access through field_class)

@ftm ftm added the enhancement New feature, or existing feature improvement label Jun 12, 2018
@ftm
Copy link
Contributor

ftm commented Jun 12, 2018

I like this idea, if you can fix the merge conflict and match the project code style (i.e. use check_validators not checkValidators) I'm up for merging it

@ftm ftm closed this Jun 12, 2018
@ftm ftm reopened this Jun 12, 2018
@@ -154,6 +157,14 @@ def __call__(self, **kwargs):
"""
return self.meta.render_field(self, kwargs)

@classmethod
def checkValidators(cls, validators):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use snake_case names: check_validators.

@classmethod
def checkValidators(cls, validators):
if validators is not None:
for validator in list(validators):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wrap with list here?

def checkValidators(cls, validators):
if validators is not None:
for validator in list(validators):
if inspect.isclass(validator) or not hasattr(validator, '__call__'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if callable(validator) and not inspect.isclass(validator):`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps also split these checks and have different messages: "{} is not a valid validator because it is not callable." and "{} is not a valid validator because it is a class, it should be an instance."

@ftm ftm merged commit 66e3ec3 into pallets-eco:master Jun 12, 2018
@ftm
Copy link
Contributor

ftm commented Jun 12, 2018

I've completed the requested changes in a new PR #410 which I've merged

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.

3 participants