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

Error context and message #183

Merged
merged 28 commits into from May 31, 2018

Conversation

2 participants
@Gr1N
Collaborator

Gr1N commented May 26, 2018

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.

@@ -61,12 +82,83 @@ class ConfigError(RuntimeError):
pass
class Missing(ValueError):
pass
class ValueError_(ValueError):

This comment has been minimized.

@samuelcolvin

samuelcolvin May 26, 2018

Owner

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

This comment has been minimized.

@Gr1N

Gr1N May 27, 2018

Collaborator

Renamed

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

This comment has been minimized.

@samuelcolvin

samuelcolvin May 26, 2018

Owner

You could have

class _PydanticMixin:
    ...

class PydanticValueError(_PydanticMixin, ValueError):
    pass

class PydanticTypeError(_PydanticMixin, TypeError):
    pass

Or does that mess up mro below?

This comment has been minimized.

@Gr1N

Gr1N May 27, 2018

Collaborator

Updated

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

This comment has been minimized.

@samuelcolvin

samuelcolvin May 26, 2018

Owner

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

This comment has been minimized.

@samuelcolvin

samuelcolvin May 26, 2018

Owner

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

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

This comment has been minimized.

@samuelcolvin

samuelcolvin May 26, 2018

Owner

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

elif isinstance(error, list):
yield from flatten_errors(error)
else:
raise RuntimeError(f'Unknown error object: {error}')
_EXC_TYPES = {}

This comment has been minimized.

@samuelcolvin

samuelcolvin May 26, 2018

Owner

Would it be more correct to use the lru_cache decorator?

This comment has been minimized.

@Gr1N

Gr1N May 27, 2018

Collaborator

Yes, you're right

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

This comment has been minimized.

@samuelcolvin

samuelcolvin May 26, 2018

Owner

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

This comment has been minimized.

@Gr1N

Gr1N May 27, 2018

Collaborator

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').

This comment has been minimized.

@samuelcolvin

samuelcolvin May 28, 2018

Owner

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

This comment has been minimized.

@Gr1N

Gr1N May 28, 2018

Collaborator

No problem, fixed.

)
class Error:
__slots__ = 'exc_info', 'loc'
__slots__ = 'exc', 'loc'

This comment has been minimized.

@samuelcolvin

samuelcolvin May 26, 2018

Owner

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

This comment has been minimized.

@Gr1N

Gr1N May 27, 2018

Collaborator

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?

This comment has been minimized.

@samuelcolvin

samuelcolvin May 28, 2018

Owner

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

This comment has been minimized.

Owner

samuelcolvin commented May 26, 2018

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

Gr1N added some commits May 27, 2018

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

This comment has been minimized.

@samuelcolvin

samuelcolvin May 28, 2018

Owner

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?

This comment has been minimized.

@Gr1N

Gr1N May 28, 2018

Collaborator

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?

This comment has been minimized.

@samuelcolvin

samuelcolvin May 28, 2018

Owner

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.

Gr1N added some commits May 28, 2018

@Gr1N Gr1N changed the title from POC of error context and message to Error context and message May 29, 2018

Gr1N added some commits May 29, 2018

@Gr1N

This comment has been minimized.

Collaborator

Gr1N commented May 30, 2018

@samuelcolvin please review

@codecov

This comment has been minimized.

codecov bot commented May 30, 2018

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.

from decimal import Decimal
from typing import Union
__all__ = (

This comment has been minimized.

@samuelcolvin

samuelcolvin May 31, 2018

Owner

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.

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

This comment has been minimized.

@samuelcolvin

samuelcolvin May 31, 2018

Owner

one line, we have 140 line limit.

)
class Error:

This comment has been minimized.

@samuelcolvin

samuelcolvin May 31, 2018

Owner

perhaps we should rename toErrorWrapper for clarity?

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

This comment has been minimized.

@samuelcolvin

samuelcolvin May 31, 2018

Owner

should we not still use to_snake_case (perhaps with re.sub('Error$', '', s) added) where code is None?

That would save quite a few lines from errors.py and allow pydantic to better support arbitrary errors.

Gr1N added some commits May 31, 2018

@Gr1N

This comment has been minimized.

Collaborator

Gr1N commented May 31, 2018

@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 samuelcolvin merged commit 4f4e22e into samuelcolvin:master May 31, 2018

3 checks passed

codecov/project Absolute coverage decreased by -0.55% but relative coverage remained the same compared to 0698384
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

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

This was referenced Jun 2, 2018

@samuelcolvin

This comment has been minimized.

Owner

samuelcolvin commented Jun 11, 2018

fixed #162.

We need to remember to reference issues in PR comments!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment