Fix #100: track position while parsing for better errors #128
Conversation
|
Hi pdedmon, Thank you for your pull request! The Travis failure is due to some lines exceeding 80 characters. Not a huge deal. I have a pre-commit hook I use to warn me about these things but I am not sure what is the best way to share it in the repository. My initial thoughts with regards to the issues tagged I will take a closer look at this PR this week to see how I feel about your proposed solution. It may be easier to fix #100 and #77 together since they both will rely on the unmangled |
pearcedavis
force-pushed
the
decode-error-position
branch
from
43a6b7a to
3bf102a
Compare
|
No worries! I just noticed the +1 for the |
|
@pdedmon any updates on this? |
|
@gaborbernat I'd forgotten about this actually, but I might look into the conflicts and build failures later. |
- errors now indicate the position in which they occurred - the way the parser currently works makes columns hard to track, so errors will often say "column 1", when they occur elsewhere in the line - errors in multiline arrays are reported to be on the opening line, due to the current preprocessing of the input
pearcedavis
force-pushed
the
decode-error-position
branch
from
3bf102a to
3ecf210
Compare
|
I rebased and did a quick manual test (on top of the toml-test and tox tests), and it seems to be working as before. |
Codecov Report
@@ Coverage Diff @@
## master #128 +/- ##
==========================================
+ Coverage 63.85% 64.12% +0.26%
==========================================
Files 4 4
Lines 711 733 +22
==========================================
+ Hits 454 470 +16
- Misses 257 263 +6
Continue to review full report at Codecov.
|
|
@uiri does this look good? |
toml/decoder.py
Outdated
| except IndexError: | ||
| raise TomlDecodeError("Invalid escape sequence") | ||
| raise ValueError() | ||
| except (IndexError, ValueError): |
There was a problem hiding this comment.
I think this should still be an IndexError? Why the change to ValueError?
There was a problem hiding this comment.
I thought the index error was a hack to fall into this except, and that a ValueError was more appropriate for an invalid hex character.
There was a problem hiding this comment.
I suppose you are right. It doesn't really matter (to me) which error type is used but I think the except should only accept the intentional error which gets raised for an invalid hex character.
|
Hi, I think that the What is the reasoning behind changing to |
|
I inherited from |
|
Inheriting from I see that there are a lot of instances of |
|
@uiri so what changes do you request/propose? |
|
I replaced the uses of TomlDecodeError with ValueError in some of the functions, because those functions didn't have access to the decoder position information. I suppose you could continue to raise a TomlDecodeError without any information, and then augment it before re-raising, but using a ValueError made more sense to me. All of the ValueErrors that have replaced TomlDecodeErrors are caught and result in TomlDecodeErrors, so we are not letting the ValueErrors escape from the lib. |
|
Oh! I see, now, thanks for pointing out the So then I think it is just the nit around the (slightly, overly) broad Thank you again for your PR and for doing the rebase after the recent significant changes to master. |
|
No worries! I'll update that |
toml/decoder.py
Outdated
| raise ValueError() | ||
| except (IndexError, ValueError): | ||
| raise ValueError("Invalid hex character") | ||
| except IndexError: |
There was a problem hiding this comment.
Uhhh... shouldn't the raised error match the excepted error?
|
Not necessarily, why?
…On Fri, 26 Jan 2018, 22:16 Will Pearson, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In toml/decoder.py
<#128 (comment)>:
> @@ -445,8 +445,8 @@ def _load_unicode_escapes(v, hexbytes, prefix):
while i < hxblen:
try:
if not hx[i].lower() in hexchars:
- raise ValueError()
- except (IndexError, ValueError):
+ raise ValueError("Invalid hex character")
+ except IndexError:
Uhhh... shouldn't the raised error match the excepted error?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#128 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AKJWC26yCfJQ20i-ao2PSwg1J5Alnh2Aks5tOr8-gaJpZM4PqKKT>
.
|
Any post-parsing issues with decoding get bubbled back up as a ValueError which is then converted into the appropriate TomlDecodeError. Actual line number/column positioning might be ported later.
I made the
TomlDecodeErrormore like theJSONDecodeErrorfrom the stdlib json module, allowing position information to be surfaced in errors.This partially addresses #100, as line numbers are reported accurately in errors. However, column numbers are harder to track in the current code; as a result, errors will often say "column 1", when the issue occurred elsewhere in the line. Also, errors in multiline arrays report the opening line, because they are currently in-lined before parsing.