v0.5.5 Deployment - Better validation error messages #263
Conversation
v0.5.1b0 Beta Deployment - Fix pickling (?)
Improve validation error messaging
v0.5.5b0 Beta deployment - Better errors!
Codecov Report
@@ Coverage Diff @@
## master #263 +/- ##
==========================================
+ Coverage 96.3% 96.36% +0.05%
==========================================
Files 17 17
Lines 2357 2366 +9
==========================================
+ Hits 2270 2280 +10
+ Misses 87 86 -1
Continue to review full report at Codecov.
|
| except (ValueError, KeyError, TypeError): | ||
| self.error(instance, value) | ||
| except GENERIC_ERRORS as err: | ||
| if hasattr(err, 'error_tuples'): |
There was a problem hiding this comment.
can't we do this through checking the subclass rather than adding a attribution dynamically?
There was a problem hiding this comment.
That makes sense, isinstance(err, ValidationError) is better than duck typing.
| self.error(instance, value) | ||
| except GENERIC_ERRORS as err: | ||
| if hasattr(err, 'error_tuples'): | ||
| extra = '({})'.format(' & '.join( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, I agree with this. We already are keeping some error info in a structured object; totally makes sense to keep it all structured.
| self.error(instance, value) | ||
| return getattr(prop, method_name)(instance, value) | ||
| except GENERIC_ERRORS as err: | ||
| if hasattr(err, 'error_tuples'): |
There was a problem hiding this comment.
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 GENERIC_ERRORS that we know have error_tuples then we could have 2 different except blocks.
| @@ -732,7 +737,7 @@ def validate(self, instance, value): | |||
| try: | |||
| value = bool(value) | |||
| except ValueError: | |||
There was a problem hiding this comment.
Can the bool function really raises value error?
There was a problem hiding this comment.
It is a bit of an unlikely / pathological case. Technically the implementation of bool(...) checks the argument for a __bool__ magic method and calls that bound to the instance. So, you could imagine a weird implementation where someone did:
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 job = something.defer(...), and then do if job.ready: ..., where job.ready is a handle to something and overrides __bool__.
In that case, since using in a conditional causes the __bool__ method to be called, you could get a ValueError if any internal code does that. However, this is not really an implementation of the bool function, but rather specific to the implementation of the object it's receiving.
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?
There was a problem hiding this comment.
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 ValueError vs. TypeError? I think it was introduced for structural consistency (as @bsmithyman suggests).
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
| if error_messages: | ||
| extra = 'Possible explanation:' | ||
| for message in error_messages: | ||
| extra += '\n - {}'.format(message) |
There was a problem hiding this comment.
Perhaps I'm missing something as to why we need to re-raise with a merged string.
What about defining a __str__ and __unicode__ and __repr__ metthod on the custom exception classed. This these methods (one of them) merging messages in a error_tuples list.
There was a problem hiding this comment.
Yeah, this is great. Feeds back into having a structured representation rather than just a somewhat arbitrary string...
Fix functools error on serializer/deserializer
Update beta with functools.wraps fix
|
Work on structured errors, as suggested in the above reviews, is in progress here: #267 - this is a significant upgrade in error handling. However, I am going ahead with merging the existing changes for now; they are backwards compatible and include major improvements to readability of errors. |
See #261