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

Make TOMLDecodeError show 'tomli' as its module #135

Merged
merged 4 commits into from
Oct 22, 2021

Conversation

goodmami
Copy link
Contributor

The README and tests show tomli.TOMLDecodeError as the error class to catch, which I take to mean it is the public API, but what's printed is tomli._parser.TOMLDecodeError because that's the module it's defined in:

>>> import tomli
>>> tomli.loads('[foo')
Traceback (most recent call last):
  ...
tomli._parser.TOMLDecodeError: Expected "]" at the end of a table declaration (at end of document)

This PR tells the exception class to use 'tomli' as its module so the displayed exception matches the public API:

>>> tomli.loads('[foo')
Traceback (most recent call last):
  ...
tomli.TOMLDecodeError: Expected "]" at the end of a table declaration (at end of document)

The exception is still defined in its original location, so if anyone saw the original error message and tried to catch the exception from toml._parser, it would still work:

>>> tomli.TOMLDecodeError is tomli._parser.TOMLDecodeError
True
>>> try:
...     tomli.loads('[foo')
... except tomli._parser.TOMLDecodeError:
...     print('still good')
... 
still good

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2021

Codecov Report

Merging #135 (e94ea77) into master (8917fe3) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #135   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          494       495    +1     
  Branches        93        93           
=========================================
+ Hits           494       495    +1     
Impacted Files Coverage Δ
tomli/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8917fe3...e94ea77. Read the comment docs.

@goodmami
Copy link
Contributor Author

It looks like the failed builds are a pytest bug on Python 3.11 (pytest-dev/pytest#9181). #133 and #134 show the same error message.

@hukkin
Copy link
Owner

hukkin commented Oct 22, 2021

Thanks for the PR!

I think I'd slightly prefer having this done in __init__.py in the same way (and with the same comment) that the standard library does here, as I find this more part of the import path manipulation happening in __init__ rather than implementation of the class defined in _parser.py. Would you mind doing that change?

It'd also be great to have a small test that fails without this line. Just a simple assert tomli.TOMLDecodeError.__module__ == "tomli" perhaps, in test_error.py.

@goodmami
Copy link
Contributor Author

I find this more part of the import path manipulation happening in __init__ rather than implementation of the class

Good point. I agree.

It'd also be great to have a small test that fails without this line.

Done. I tested that it fails without the change and passes with it.

@hukkin hukkin merged commit 96dfe2c into hukkin:master Oct 22, 2021
@hukkin
Copy link
Owner

hukkin commented Oct 22, 2021

This is great, thanks a lot!

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.

3 participants