-
Notifications
You must be signed in to change notification settings - Fork 9
v0.5.5 Deployment - Better validation error messages #263
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
Changes from all commits
c464c06
e34b2f6
7853d62
7665f3d
acf37d1
a9536f3
03b5cc2
076f0e2
dd5bf45
2f81a37
32713b3
52a53bc
76455fd
2a20f8d
8297f63
d8dc762
292b272
5286ee4
9f285b3
ad343ce
4c91459
9d3f1ce
f3e75a4
559a9f7
5dd72d0
fee4b30
fe28b97
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| [bumpversion] | ||
| current_version = 0.5.4 | ||
| current_version = 0.5.5 | ||
| files = properties/__init__.py setup.py docs/conf.py | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
|
|
||
| from six import PY2 | ||
|
|
||
| from .base import HasProperties, equal | ||
| from .base import GENERIC_ERRORS, HasProperties, equal | ||
| from .. import basic | ||
| from .. import utils | ||
|
|
||
|
|
@@ -101,8 +101,14 @@ def validate(self, instance, value): | |
| if isinstance(value, dict): | ||
| return self.instance_class(**value) | ||
| return self.instance_class(value) | ||
| except (ValueError, KeyError, TypeError): | ||
| self.error(instance, value) | ||
| except GENERIC_ERRORS as err: | ||
| if hasattr(err, 'error_tuples'): | ||
| extra = '({})'.format(' & '.join( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally I find this a little odd, below we re-raise this maybe we can find a way to store this list of tuples on the re-raised error.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I agree with this. We already are keeping some error info in a structured object; totally makes sense to keep it all structured. |
||
| err_tup.message for err_tup in err.error_tuples | ||
| )) | ||
| else: | ||
| extra = '' | ||
| self.error(instance, value, extra=extra) | ||
|
|
||
| def assert_valid(self, instance, value=None): | ||
| """Checks if valid, including HasProperty instances pass validation""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,8 @@ | |
|
|
||
| from six import PY2 | ||
|
|
||
| from ..base import GENERIC_ERRORS, HasProperties, Instance | ||
| from .base import GENERIC_ERRORS, HasProperties | ||
| from .instance import Instance | ||
| from .. import basic | ||
| from .. import utils | ||
|
|
||
|
|
@@ -160,14 +161,32 @@ def _unused_default_warning(self): | |
| warn('Union prop default ignored: {}'.format(prop.default), | ||
| RuntimeWarning) | ||
|
|
||
| def validate(self, instance, value): | ||
| """Check if value is a valid type of one of the Union props""" | ||
| def _try_prop_method(self, instance, value, method_name): | ||
| """Helper method to perform a method on each of the union props | ||
|
|
||
| This method gathers all errors and returns them at the end | ||
| if the method on each of the props fails. | ||
| """ | ||
| error_messages = [] | ||
| for prop in self.props: | ||
| try: | ||
| return prop.validate(instance, value) | ||
| except GENERIC_ERRORS: | ||
| continue | ||
| self.error(instance, value) | ||
| return getattr(prop, method_name)(instance, value) | ||
| except GENERIC_ERRORS as err: | ||
| if hasattr(err, 'error_tuples'): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above checking for an attribute on an instance of a class is, in my view, not the cleanest way. if we could have some subclasses of our |
||
| error_messages += [ | ||
| err_tup.message for err_tup in err.error_tuples | ||
| ] | ||
| if error_messages: | ||
| extra = 'Possible explanation:' | ||
| for message in error_messages: | ||
| extra += '\n - {}'.format(message) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps I'm missing something as to why we need to re-raise with a merged string. What about defining a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is great. Feeds back into having a structured representation rather than just a somewhat arbitrary string... |
||
| else: | ||
| extra = '' | ||
| self.error(instance, value, extra=extra) | ||
|
|
||
| def validate(self, instance, value): | ||
| """Check if value is a valid type of one of the Union props""" | ||
| return self._try_prop_method(instance, value, 'validate') | ||
|
|
||
| def assert_valid(self, instance, value=None): | ||
| """Check if the Union has a valid value""" | ||
|
|
@@ -178,19 +197,7 @@ def assert_valid(self, instance, value=None): | |
| value = instance._get(self.name) | ||
| if value is None: | ||
| return True | ||
| for prop in self.props: | ||
| try: | ||
| return prop.assert_valid(instance, value) | ||
| except GENERIC_ERRORS: | ||
| continue | ||
| message = ( | ||
| 'The "{name}" property of a {cls} instance has not been set ' | ||
| 'correctly'.format( | ||
| name=self.name, | ||
| cls=instance.__class__.__name__ | ||
| ) | ||
| ) | ||
| raise utils.ValidationError(message, 'invalid', self.name, instance) | ||
| return self._try_prop_method(instance, value, 'assert_valid') | ||
|
|
||
| def serialize(self, value, **kwargs): | ||
| """Return a serialized value | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,6 @@ | |
|
|
||
| import collections | ||
| import datetime | ||
| from functools import wraps | ||
| import math | ||
| import random | ||
| import re | ||
|
|
@@ -45,7 +44,6 @@ def accept_kwargs(func): | |
| functions always receive kwargs from serialize, but by using this, | ||
| the original functions may simply take a single value. | ||
| """ | ||
| @wraps(func) | ||
| def wrapped(val, **kwargs): | ||
| """Perform a function on a value, ignoring kwargs if necessary""" | ||
| try: | ||
|
|
@@ -339,14 +337,19 @@ def error(self, instance, value, error_class=None, extra=''): | |
| prefix = prefix + ' of a {cls} instance'.format( | ||
| cls=instance.__class__.__name__, | ||
| ) | ||
| print_value = repr(value) | ||
| if len(print_value) > 107: | ||
| print_value = '{} ... {}'.format( | ||
| print_value[:50], print_value[-50:] | ||
| ) | ||
| message = ( | ||
| '{prefix} must be {info}. A value of {val!r} {vtype!r} was ' | ||
| 'specified. {extra}'.format( | ||
| '{prefix} must be {info}. An invalid value of {val} {vtype} was ' | ||
| 'specified.{extra}'.format( | ||
| prefix=prefix, | ||
| info=self.info or 'corrected', | ||
| val=value, | ||
| val=print_value, | ||
| vtype=type(value), | ||
| extra=extra, | ||
| extra=' {}'.format(extra) if extra else '', | ||
| ) | ||
| ) | ||
| if issubclass(error_class, ValidationError): | ||
|
|
@@ -732,7 +735,7 @@ def validate(self, instance, value): | |
| try: | ||
| value = bool(value) | ||
| except ValueError: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the bool function really raises value error?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a bit of an unlikely / pathological case. Technically the implementation of This seems really odd at first, but maybe you could imagine a use case like a future object or a deferred job ID. You do a In that case, since using in a conditional causes the I'm not sure there is a lot of benefit to catching this, other than that it is structurally consistent with the other property implementations. I guess it doesn't hurt either. Thoughts @fwkoch?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @hishnash and @bsmithyman - I agree that it doesn't make a ton of sense to catch this. It will only come up in strange situations, and in that case, what is special about For the sake of this PR, I will leave it (outside the scope of the current changes, not quite backwards compatible), but it's worth addressing in the future. #264 |
||
| self.error(instance, value) | ||
| self.error(instance, value, extra='Cannot cast to boolean.') | ||
| if not isinstance(value, BOOLEAN_TYPES): | ||
| self.error(instance, value) | ||
| return value | ||
|
|
@@ -765,7 +768,7 @@ def _in_bounds(prop, instance, value): | |
| (prop.min is not None and value < prop.min) or | ||
| (prop.max is not None and value > prop.max) | ||
| ): | ||
| prop.error(instance, value) | ||
| prop.error(instance, value, extra='Not within allowed range.') | ||
|
|
||
|
|
||
| class Integer(Boolean): | ||
|
|
@@ -811,9 +814,13 @@ def validate(self, instance, value): | |
| try: | ||
| intval = int(value) | ||
| if not self.cast and abs(value - intval) > TOL: | ||
| self.error(instance, value) | ||
| self.error( | ||
| instance=instance, | ||
| value=value, | ||
| extra='Not within tolerance range of {}.'.format(TOL), | ||
| ) | ||
| except (TypeError, ValueError): | ||
| self.error(instance, value) | ||
| self.error(instance, value, extra='Cannot cast to integer.') | ||
| _in_bounds(self, instance, intval) | ||
| return intval | ||
|
|
||
|
|
@@ -861,9 +868,13 @@ def validate(self, instance, value): | |
| try: | ||
| floatval = float(value) | ||
| if not self.cast and abs(value - floatval) > TOL: | ||
| self.error(instance, value) | ||
| self.error( | ||
| instance=instance, | ||
| value=value, | ||
| extra='Not within tolerance range of {}.'.format(TOL), | ||
| ) | ||
| except (TypeError, ValueError): | ||
| self.error(instance, value) | ||
| self.error(instance, value, extra='Cannot cast to float.') | ||
| _in_bounds(self, instance, floatval) | ||
| return floatval | ||
|
|
||
|
|
@@ -907,7 +918,11 @@ def validate(self, instance, value): | |
| abs(value.real - compval.real) > TOL or | ||
| abs(value.imag - compval.imag) > TOL | ||
| ): | ||
| self.error(instance, value) | ||
| self.error( | ||
| instance=instance, | ||
| value=value, | ||
| extra='Not within tolerance range of {}.'.format(TOL), | ||
| ) | ||
| except (TypeError, ValueError, AttributeError): | ||
| self.error(instance, value) | ||
| return compval | ||
|
|
@@ -1012,7 +1027,7 @@ def validate(self, instance, value): | |
| if not isinstance(value, string_types): | ||
| self.error(instance, value) | ||
| if self.regex is not None and self.regex.search(value) is None: #pylint: disable=no-member | ||
| self.error(instance, value) | ||
| self.error(instance, value, extra='Regex does not match.') | ||
| value = value.strip(self.strip) | ||
| if self.change_case == 'upper': | ||
| value = value.upper() | ||
|
|
@@ -1153,7 +1168,7 @@ def validate(self, instance, value): #pyli | |
| test_val = val if self.case_sensitive else [_.upper() for _ in val] | ||
| if test_value == test_key or test_value in test_val: | ||
| return key | ||
| self.error(instance, value) | ||
| self.error(instance, value, extra='Not an available choice.') | ||
|
|
||
|
|
||
| class Color(Property): | ||
|
|
@@ -1226,11 +1241,19 @@ def validate(self, instance, value): | |
| if isinstance(value, datetime.datetime): | ||
| return value | ||
| if not isinstance(value, string_types): | ||
| self.error(instance, value) | ||
| self.error( | ||
| instance=instance, | ||
| value=value, | ||
| extra='Cannot convert non-strings to datetime.', | ||
| ) | ||
| try: | ||
| return self.from_json(value) | ||
| except ValueError: | ||
| self.error(instance, value) | ||
| self.error( | ||
| instance=instance, | ||
| value=value, | ||
| extra='Invalid format for converting to datetime.', | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def to_json(value, **kwargs): | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we do this through checking the subclass rather than adding a attribution dynamically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense,
isinstance(err, ValidationError)is better than duck typing.