v0.5.5 Deployment - Better validation error messages by fwkoch · Pull Request #263 · seequent/properties

fwkoch · 2018-11-30T18:53:56Z

v0.5.1b0 Beta Deployment - Fix pickling (?)

Improve validation error messaging

v0.5.5b0 Beta deployment - Better errors!

codecov · 2018-11-30T18:56:04Z

Codecov Report

Merging #263 into master will increase coverage by 0.05%.
The diff coverage is 91.83%.

@@            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

Impacted Files	Coverage Δ
properties/base/containers.py	`89.11% <100%> (+0.52%)`	⬆️
properties/__init__.py	`76% <100%> (ø)`	⬆️
properties/math.py	`98.55% <100%> (+0.01%)`	⬆️
properties/extras/web.py	`100% <100%> (ø)`	⬆️
properties/base/instance.py	`96.7% <100%> (+0.11%)`	⬆️
properties/basic.py	`97.07% <78.57%> (ø)`	⬆️
properties/base/union.py	`97.27% <94.11%> (-0.63%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1546b1b...fe28b97. Read the comment docs.

hishnash

I'm not a fan of catching value errors is it possible to be more particular in these cases?

hishnash · 2018-12-04T01:36:57Z

properties/base/instance.py

-        except (ValueError, KeyError, TypeError):
-            self.error(instance, value)
+        except GENERIC_ERRORS as err:
+            if hasattr(err, 'error_tuples'):


can't we do this through checking the subclass rather than adding a attribution dynamically?

That makes sense, isinstance(err, ValidationError) is better than duck typing.

hishnash · 2018-12-04T01:37:18Z

properties/base/instance.py

-            self.error(instance, value)
+        except GENERIC_ERRORS as err:
+            if hasattr(err, 'error_tuples'):
+                extra = '({})'.format(' & '.join(


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.

Yeah, I agree with this. We already are keeping some error info in a structured object; totally makes sense to keep it all structured.

hishnash · 2018-12-04T01:38:23Z

properties/base/union.py

-        self.error(instance, value)
+                return getattr(prop, method_name)(instance, value)
+            except GENERIC_ERRORS as err:
+                if hasattr(err, 'error_tuples'):


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.

hishnash · 2018-12-04T01:40:27Z

properties/basic.py

@@ -732,7 +737,7 @@ def validate(self, instance, value):
            try:
                value = bool(value)
            except ValueError:


Can the bool function really raises value error?

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?

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

hishnash · 2018-12-04T17:16:11Z

properties/base/union.py

+        if error_messages:
+            extra = 'Possible explanation:'
+            for message in error_messages:
+                extra += '\n    - {}'.format(message)


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.

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

fwkoch · 2019-01-16T01:32:10Z

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.

fwkoch added 19 commits June 7, 2018 10:39

Merge pull request #244 from seequent/dev

c464c06

v0.5.1b0 Beta Deployment - Fix pickling (?)

Propagate validation errors when instance properties are set from dicts

e34b2f6

Merge branch 'master' into feat/propagate_errors

7853d62

Print truncated representation of bad values in error messages

7665f3d

Add extra info to most error messages

acf37d1

Remove unused import

a9536f3

Fix strange import paths

03b5cc2

Break out union property error collection to separate method

076f0e2

Revert a few redundant 'extra' error messages

dd5bf45

Add test for truncated error messages

2f81a37

Tidy up vector array error messages for string representations

32713b3

A couple more tiny error tests

52a53bc

Add ValueError to possible exceptions on dictionary coercion

76455fd

Merge pull request #261 from seequent/feat/more_error_info

2a20f8d

Improve validation error messaging

Merge branch 'master' into beta

8297f63

Merge branch 'beta' into dev

d8dc762

Bump version: 0.5.4 → 0.5.5b0

292b272

Fix potentially cyclic import

5286ee4

Merge pull request #262 from seequent/dev

9f285b3

v0.5.5b0 Beta deployment - Better errors!

hishnash reviewed Dec 4, 2018

View reviewed changes

fwkoch added 5 commits January 8, 2019 08:50

Do not use functools.wrap for ser/deser functions

ad343ce

Add deserializer test that would fail previously

4c91459

Merge branch 'dev' into fix/functools_error

9d3f1ce

Update tests for pypng updates

f3e75a4

Merge pull request #265 from seequent/fix/functools_error

559a9f7

Fix functools error on serializer/deserializer

fwkoch mentioned this pull request Jan 16, 2019

[WIP] Structured errors #267

Open

fwkoch added 2 commits January 15, 2019 17:56

Merge branch 'beta' into dev

5dd72d0

Merge pull request #268 from seequent/dev

fee4b30

Update beta with functools.wraps fix

Bump version: 0.5.5b0 → 0.5.5

fe28b97

fwkoch merged commit b2014e3 into master Jan 16, 2019

Conversation

fwkoch commented Nov 30, 2018

Uh oh!

codecov bot commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hishnash left a comment • edited by rowanc1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hishnash Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

fwkoch Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

hishnash Dec 4, 2018 • edited by rowanc1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fwkoch Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

hishnash Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hishnash Dec 4, 2018 • edited by rowanc1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsmithyman Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

fwkoch Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

hishnash Dec 4, 2018 • edited by rowanc1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fwkoch Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

fwkoch commented Jan 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 30, 2018 •

edited

Loading

hishnash left a comment •

edited by rowanc1

Loading

hishnash Dec 4, 2018 •

edited by rowanc1

Loading

hishnash Dec 4, 2018 •

edited

Loading

hishnash Dec 4, 2018 •

edited by rowanc1

Loading

hishnash Dec 4, 2018 •

edited by rowanc1

Loading