-
Notifications
You must be signed in to change notification settings - Fork 145
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
Add new exception ExtraItemsError #241
Conversation
class ExtraItemsError(Invalid): | ||
def __init__(self, node, extras, msg=None): | ||
if msg is None: | ||
msg = _('Unrecognized items') |
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.
You can fix the coverage failure by moving the _('Unrecognize items')
into the function signature itself. E.g.:
class ExtraItemsError(Invalid):
def __init__(self, node, extras, msg=_('Unrecognized items')):
super(ExtraItemsError, self).__init__(node, msg)
self.extras = extras
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.
In this case it will be evaluated on module import time. Is it not too early? I think many program make message translation configuration on more later stage.
LGTM, assuming the coverage failure is fixed. |
This needs docs updated and a changelog entry. For example Line 553 in f7dabb5
Also please add your name to CONTRIBUTORS.txt. |
This allow caller to analise errors from extra fields programmatically. Without this error type to know which fields treated as "extra" caller must parse error string.
Is it ok now? |
) | ||
raise ExtraItemsError( | ||
node, value, | ||
msg=_('Unrecognized keys in mapping: "${val}"', |
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.
nit: Unknown
looks more appropriate than Unrecognized
Great and useful feature! I had troubles understanding the name of the exception. Couldn't we find something with |
As for me, UnknownKeyError is too common. But I don't insist on my variant, so it up to community. |
In case anyone else is thinking the same thing I first thought... When I first saw this I thought "what's the point?". If you get random keys you didn't expect then there's not really any way to recover from that so just throw the exception and be done with it. And while that's still true, what @surabujin is trying to do is make the exception into a format that can be transformed into errors that conform to some convention in other libraries. Specifically, he's trying to throw Cornice JSON errors that outline more specifically where an error occurred in the REST service (if I'm understanding his PR to Cornice properly). |
@tisdall yep - you understand everything right. I will need some more changes to colander, if this and cornice PR will be approved. |
Some bikeshedding, since another conflict-resolving merge is needed... why not UnexpectedItemError ? |
I'm going to rename this to |
Changed my mind for the last time. Error will be |
+1 for UnsupportedFields! |
This allow caller to analise errors from extra fields programmatically.
Without this error type to know which fields treated as "extra" caller
must parse error string.