-
Notifications
You must be signed in to change notification settings - Fork 156
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 encoder and decoder exception handling #248
base: master
Are you sure you want to change the base?
Conversation
dataclasses_json/core.py
Outdated
@@ -111,7 +111,7 @@ def _encode_overrides(kvs, overrides, encode_json=False): | |||
if encoder is not None: | |||
try: | |||
v = encoder(v) | |||
except: | |||
except: # noqa: E722 |
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.
except: # noqa: E722 | |
except Exception as e: # noqa: E722 | |
raise ValueError(f"Encoder encountered an error with field '{k}'") from e |
If you make it more readable you might as well include the original error message in the stack trace :)
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.
Hi @RunOrVeith - I've fixed the bare except. In Python 3, the entire stack trace is presented to the user when raising an exception inside the except
block. By using from e
, the error message is changed to say The above exception was the direct cause of the following exception
. Reading the output, it will make it sound like dataclass_json
caused the encoder
function to have an exception, which is incorrect.
To make this read properly (since dataclass_json
raise an exception that was directly caused by the encoder
), we would need to do raise e from ValueError()
. The problem this has is that it puts the explanation that helps the user at the top of the stack trace, and forces someone to scroll up and scan the trace.
Personally, I prefer the way it currently is as it both displays the entire trace including previous exceptions, but also places the most important piece at the bottom of the error output, without confusing language. Your thoughts on this?
As it currently is, the output will look like this:
>>> b.to_json()
Traceback (most recent call last):
File "/Users/mike/code/dataclasses-json/dataclasses_json/core.py", line 113, in _encode_overrides
v = encoder(v)
ValueError: invalid literal for int() with base 10: 'foo'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/mike/code/dataclasses-json/dataclasses_json/api.py", line 50, in to_json
return json.dumps(self.to_dict(encode_json=False),
File "/Users/mike/code/dataclasses-json/dataclasses_json/api.py", line 86, in to_dict
return _asdict(self, encode_json=encode_json)
File "/Users/mike/code/dataclasses-json/dataclasses_json/core.py", line 349, in _asdict
encode_json=encode_json)
File "/Users/mike/code/dataclasses-json/dataclasses_json/core.py", line 116, in _encode_overrides
f"Encoder encountered an error with field '{k}'"
ValueError: Encoder encountered an error with field 'time'
dataclasses_json/core.py
Outdated
@@ -201,10 +201,10 @@ def _decode_dataclass(cls, kvs, infer_missing): | |||
init_kwargs[field.name] = overrides[field.name].decoder( | |||
field_value | |||
) | |||
except: | |||
except: # noqa: E722 |
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.
I would suggest to do the same as above here as well (raise from the original exception)
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.
I would suggest raising from the original exception to get an even better overview over the issue that occured
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.
Lgtm, please sync with main and I'll merge :)
@@ -107,7 +107,14 @@ def _encode_overrides(kvs, overrides, encode_json=False): | |||
k = letter_case(k) if letter_case is not None else k | |||
|
|||
encoder = overrides[original_key].encoder | |||
v = encoder(v) if encoder is not None else v | |||
# v = encoder(v) if encoder is not None else v |
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.
Remove commented code
@mbrancato hello from when you were 3 years younger :) any chance you could look at CI and make it green? |
Hi @lidatong - This PR is to help with implementation issues I've run into by reporting some errors that will help users quickly find the cause of a problem. Let me know if anything here might be of concern or need additional improvements.