Error context and message #183

Gr1N · 2018-05-26T11:36:18Z

This is an implementation of my vision of how to pass error context and build error message.

Also I'm thinking about redefining error template on config level, something like:

class Config:
    error_tmpls = {
        get_exc_type(DecimalError): 'custom error message',
    }

It's easy to implement we just need to pass config into exceptions.Error and improve msg property logic.

@samuelcolvin what do you think about PR and idea with config?

samuelcolvin

Overall I think this looks like a pretty good start.

Let's have a separate module for these errors (sorry to change my mind) then we can simply do

from pydantic import errors

...

raise errors.DecimalError()

Combining a few of my comments, here is my suggestion for the rough layout of errors.py:

class _PydanticMixin:
    default_msg_template = None
    # __slots__ ?

    def __init__(self, msg_template, **ctx):
        self.ctx = ctx or None
        self.msg_template = msg_template or self.default_msg_template
        self.loc = None
        super().__init__()
    
    @property
    def msg(self):
        return str(self)

    @property
    def type_(self):
        return get_exc_type(self)

    def as_dict(self, *, loc_prefix=None):
        loc = self.loc if loc_prefix is None else loc_prefix + self.loc

        d = {
            'loc': loc,
            'msg': self.msg,
            'type': self.type_,
        }
        if self.ctx is not None:
            d['ctx'] = self.ctx
        return d

    def __str__(self) -> str:
        return self.msg_template.format(**self.ctx or {})


class PydanticValueError(_PydanticMixin, ValueError):
    pass


class PydanticTypeError(_PydanticMixin, TypeError):
    pass


class MissingError(PydanticValueError):
    default_msg_template = 'field required'


class ExtraError(PydanticValueError):
    default_msg_template = 'extra fields not permitted'


class DecimalError(PydanticTypeError):
    default_msg_template = 'value is not a valid decimal'


class DecimalIsNotFiniteError(DecimalError):
    default_msg_template = 'value is not a valid decimal'

...

error_tmpls (let's call them error_templates for clarity) is a good idea, but perhaps we should set them when serialising the ValidationError rather than in config. In my experience I often have one place where I serialised ValidationError from lots of different models, eg. here I guess best to allow both.

samuelcolvin · 2018-05-26T11:40:46Z

pydantic/exceptions.py


-class Missing(ValueError):
-    pass
+class ValueError_(ValueError):


I get your idea here, but let's call this PydanticValueError as it's more descriptive, same for PydanticTypeError

samuelcolvin · 2018-05-26T11:42:34Z

pydantic/exceptions.py

-    pass
+class ValueError_(ValueError):
+    def __init__(self, msg_tmpl, **ctx):
+        self.ctx = ctx or None


You could have

class _PydanticMixin: ... class PydanticValueError(_PydanticMixin, ValueError): pass class PydanticTypeError(_PydanticMixin, TypeError): pass

Or does that mess up mro below?

samuelcolvin · 2018-05-26T11:45:07Z

pydantic/exceptions.py

+class ValueError_(ValueError):
+    def __init__(self, msg_tmpl, **ctx):
+        self.ctx = ctx or None
+        self.msg_tmpl = msg_tmpl


maybe good to have

class ... default_msg_tmpl = None def __init__...: self.msg_tmpl = msg_tmpl or self.default_msg_tmpl

Then many of the errors below can be simplified to

class ExtraError(ValueError_): default_msg_tmpl = 'extra fields not permitted

also can we use template not tmpl, it could be confusing and it's not that much longer.

samuelcolvin · 2018-05-26T11:46:31Z

pydantic/exceptions.py

+        super().__init__('value is not a valid decimal')
+
+
+class DecimalIsNotFiniteError(ValueError_):


I guess all these other decimal errors should inherit from DecimalError?

samuelcolvin · 2018-05-26T11:48:46Z

pydantic/exceptions.py

            raise RuntimeError(f'Unknown error object: {error}')
+
+
+_EXC_TYPES = {}


Would it be more correct to use the lru_cache decorator?

Yes, you're right

samuelcolvin · 2018-05-26T11:53:19Z

pydantic/exceptions.py

+            'loc': loc,
+            'msg': self.msg,
+            'type': self.type_,
+            'ctx': self.ctx,


Let's not include ctx if it's None.

Why? For me it's better to return all keys with useful defaults, like None if there are no context. In this case user see that ctx is possible, but is empty now. Also from my point of view, better to write code like ctx = error.dict()['ctx'] rather than ctx = error.dict().get('ctx').

I think it's better to keep errors respones as short as possible. I understand your point of you but I'd still rather removing ctx if it's None

No problem, fixed.

samuelcolvin · 2018-05-26T12:27:38Z

pydantic/exceptions.py


 class Error:
-    __slots__ = 'exc_info', 'loc'
+    __slots__ = 'exc', 'loc'


Perhaps we can get rid of Error altogether and store loc on the Exception?

I don't think that we need to get rid of Error also I get your idea with _PydanticMixin which contains all about error (ctx, msg, loc, ...), but I don't think that this is a good idea.

To my mind this is not a good idea because:

Error and ValidationError are wrappers around pydantic's validation logic, they hides inside errors representation and things like loc;

moving logic into _PydanticMixin will broke your idea with TypeError and ValueError which can be used by users to raise errors in user's validators;

in case if we want to store loc inside error as you proposed we need to rewrite a huge part because for now exceptions know nothing about where they raised.

I think we need to keep Error and ValidationError, but we need to change some things, maybe just naming. Also I want to keep Error because it's a good wrapper/container to hide validation logic from users and it's also helps us to change anything inside without introducing breaking changes.

I propose:

move all errors like MissingError, ExtraError, DecimalError to errors.py module;

rename Error to FieldError, because it's a error container which knows all about field error (exception and location) and it's a good place to work with config to get custom message templates;

rename exceptions.py to something more descriptive? like error_wrappers.py?

moving logic into _PydanticMixin will break your idea with TypeError and ValueError which can be used by users to raise errors in user's validators

Very good point, let's Keep Error

move all errors like MissingError, ExtraError, DecimalError to errors.py module;

Okay

rename Error to FieldError, because it's a error container which knows all about field error (exception and location) and it's a good place to work with config to get custom message templates;

I think Error is fine.

rename exceptions.py to something more descriptive? like error_wrappers.py?

Okay

samuelcolvin · 2018-05-26T17:25:41Z

we should use dict() not as_dict() to match the rest of pydantic.

… bit

samuelcolvin · 2018-05-28T17:49:20Z

pydantic/errors.py

+
+
+class DecimalError(PydanticTypeError):
+    msg_template = 'value is not a valid decimal'


from gitter you said

In your example DecimalError inherited from TypeError, so if I inherit all decimal errors from DecimalError they will be type errors not value errors as now. Is it okay?

You're right, that's not ok, so the other decimal related exceptions below can't inherit from this DecimalError.

However we want the end type on the exception to be value_error.decimal.is_not_finite rather than value_error.decimal_is_not_finite. etc.

The way to do this is to add display_name (or similar) to exceptions:

class DecimalIsNotFiniteError(PydanticValueError): display_name = 'decimal.not_finite_error'

Then in exceptions._get_exc_bases do

name = getattr(b, 'display_name', None) or b.__name__ yield name.replace('Error', '')

does that make sense?

If we want to introduce display_name then I think we can simplify get_exc_type func logic to something like this:

def get_exc_type(exc: Exception) -> str: exc = type(exc) if issubclass(exc, TypeError): type_ = 'type_error' elif issubclass(exc, ValueError): type_ = 'value_error' else: RuntimeError(f'Unsupported exception: {exc}') display_name = getattr(exc, 'display_name', None) if display_name is not None: type_ = f'{type_}.{display_name}' return type_

And we can remove to_snake_case and all logic with MRO. Also I think it's more easy to understand and support. What do you think?

I agree except that f'{type_}.{display_name}' I think will be wrong because to get decimal.not_finite_error and decimal.max_digits_exceeded you would need duplicate exception names

But maybe best for you to implement it and see how you go.

…#174)

Gr1N · 2018-05-30T18:11:01Z

@samuelcolvin please review

codecov · 2018-05-30T18:11:18Z

Codecov Report

Merging #183 into master will decrease coverage by 0.55%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #183      +/-   ##
==========================================
- Coverage     100%   99.44%   -0.56%     
==========================================
  Files           9        9              
  Lines         995     1088      +93     
  Branches      206      217      +11     
==========================================
+ Hits          995     1082      +87     
- Misses          0        4       +4     
- Partials        0        2       +2

samuelcolvin

This looks awesome, I'm really excited to get it merged and deployed.

Just a few small things, also could you update HISTORY.rst, then I'll merge.

samuelcolvin · 2018-05-31T10:23:51Z

pydantic/errors.py

+from decimal import Decimal
+from typing import Union
+
+__all__ = (


I think this will lead to problems when people (eg. me) add errors and forget to update __all__, let's remove __all__.

If we're really worried about exposing Decimal etc. we could even import them as from decimal import Decimal as _Decimal, but I don't think it's necessary.

samuelcolvin · 2018-05-31T10:24:19Z

pydantic/errors.py

+        return self.msg_template.format(**self.ctx or {})
+
+
+class PydanticTypeError(PydanticErrorMixin,


one line, we have 140 line limit.

samuelcolvin · 2018-05-31T10:26:21Z

pydantic/error_wrappers.py

+)
+
+
+class Error:


perhaps we should rename toErrorWrapper for clarity?

samuelcolvin · 2018-05-31T10:34:11Z

pydantic/error_wrappers.py

+
+    code = getattr(exc, 'code', None)
+    if code is not None:
+        type_ = f'{type_}.{code}'


Gr1N · 2018-05-31T13:29:33Z

@samuelcolvin check again please (:

And please don't deploy new version, because I need to send one more PR with huge update for documentation, I would like to cover all about errors.

Also I think we need to fix two moments:

samuelcolvin · 2018-06-11T10:18:51Z

fixed #162.

We need to remember to reference issues in PR comments!

* working on PydanticValueError, fix pydantic#183 * rust class working * linting * add benchmarks * improvements to PydanticValueError * custom int context value processing * fix tests

POC of error context and message

c1110f0

samuelcolvin reviewed May 26, 2018

View reviewed changes

Gr1N added 9 commits May 27, 2018 22:17

Move type errors to the errors.py module; Change errors interface a…

8a25c51

… bit

Merge branch 'master' into errors-context

b14c7bd

Rename .as_dict() to .dict()

f8e4478

Fix PydanticErrorMixin constructor

9692994

Rename exceptions.py to error_wrappers.py

4ab5e69

Do not include nullable ctx

15cd4e1

Fix tests

ed6d84e

Added int_validator; Added IntegerError

9e2ed7f

Added float_validator; Added FloatError

6447783

samuelcolvin reviewed May 28, 2018

View reviewed changes

Gr1N added 4 commits May 28, 2018 22:00

Get rid of __mro__ in prior of exc.code

01a20e5

Removed min_number_size and max_number_size from config (pydantic…

ecb2ecc

…#174)

Added NumberMinSizeError and NumberMaxSizeError

a2cc0a9

Added NoneIsNotAllowedError

ef07e61

Gr1N changed the title ~~POC of error context and message~~ Error context and message May 29, 2018

Gr1N added 12 commits May 29, 2018 13:31

Added EnumError

04d72e9

Added path_validator; Added PathError

bc3bed8

Added DictError

a677a7e

Added ListError

89804fb

Added TupleError

9e82397

Added SetError

6c2da85

Added datetime related errors

45f49c1

Added bytes and str related errors

95b710f

Added SequenceError

af3d24f

Improved code coverage

c5c906e

Display error context in string representation of validation error

6e62458

Redefine error message templates using config

9b726ee

samuelcolvin requested changes May 31, 2018

View reviewed changes

Gr1N added 2 commits May 31, 2018 16:14

Review fixes

446cc76

Updated changelog

81df1c2

samuelcolvin merged commit 4f4e22e into pydantic:master May 31, 2018

Gr1N deleted the errors-context branch May 31, 2018 13:37

This was referenced Jun 2, 2018

Cleanup 183 #187

Merged

Version 1 Release #143

Closed

		super().__init__('value is not a valid decimal')


		class DecimalIsNotFiniteError(ValueError_):

		raise RuntimeError(f'Unknown error object: {error}')


		_EXC_TYPES = {}



		class DecimalError(PydanticTypeError):
		msg_template = 'value is not a valid decimal'

		return self.msg_template.format(**self.ctx or {})


		class PydanticTypeError(PydanticErrorMixin,

Uh oh!

Error context and message #183

Error context and message #183

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants