Skip to content

Add encoder and decoder exception handling #248

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.

Sign up for GitHub

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

Open
mbrancato wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add encoder and decoder exception handling #248

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6672276
Add encoder and decoder exception handling
mbrancato Sep 25, 2020
182f6c4
Fix linting
mbrancato Sep 26, 2020
e7837a9
Fix bare except
mbrancato Sep 27, 2020
376d843
Merge branch 'master' into add_enc_dec_errors
george-zubrienko Jun 16, 2023
005951e
Merge branch 'master' into add_enc_dec_errors
george-zubrienko Jul 1, 2023
e0c99a7
Merge branch 'master' into add_enc_dec_errors
george-zubrienko Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions dataclasses_json/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@RunOrVeith RunOrVeith Sep 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
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 :)

@mbrancato mbrancato Sep 27, 2020

Copy link
Copy Markdown
Author

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'

raise ValueError(
f"Encoder encountered an error with field '{k}'"
)
Expand Down Expand Up @@ -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

@RunOrVeith RunOrVeith Sep 27, 2020

Copy link
Copy Markdown
Contributor

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)

raise ValueError(
f"Decoder encountered an error with field '{field.name}'"
f"in '{cls.__name__}'"
f"Decoder encountered an error with field "
f"'{field.name}' in '{cls.__name__}'"
)
elif is_dataclass(field_type):
# FIXME this is a band-aid to deal with the value already being
Expand Down