Skip to content
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

Added custom exception (inheriting from KeyError) when deserialization fails #207

Open
wants to merge 2 commits into
base: feat/207
Choose a base branch
from

Conversation

eblis
Copy link

@eblis eblis commented Apr 9, 2020

I have added a quickly put together patch that helps with issue #206, it will raise a custom exception (that inherits from KeyError from clients that were catching that exception) and reports the actual object that failed and the field it failed for.

@eblis
Copy link
Author

eblis commented Apr 9, 2020

Hmm, why did the second try get cancelled? I think it should have passed the linter this time

@eblis eblis force-pushed the deserialization-exception branch from 177ef80 to 64cd7b1 Compare April 15, 2020 06:03
@eblis eblis force-pushed the deserialization-exception branch from 64cd7b1 to 7031df5 Compare April 15, 2020 06:13
@lidatong lidatong self-requested a review April 21, 2020 23:39
@@ -0,0 +1,3 @@
class DataclassSerializationException(KeyError):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps class DataclassJsonError(Exception) instead, to make it clear it's coming from this package, not dataclasses itself -- user-defined exceptions typically inherit from Exception

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird not inheriting from Exception but I wanted to inherit from KeyError so that any client code that might have caught the old exception would still catch the new one.
I feel like inheriting from Exception would be a bit of a breaking change .. fine by me, but not sure if everyone would feel that way.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha, that's a good thought and thanks for clarifying

Copy link
Owner

@lidatong lidatong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the contribution! better errors is always great, and i appreciate the contribution a lot

@lidatong lidatong changed the base branch from master to feat/207 April 21, 2020 23:45
@lidatong
Copy link
Owner

lidatong commented Apr 21, 2020

see #206 (comment)

…lassJson).

Added base exception, but currently not inheriting from that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants