prevent unnessary b' prefix in parse_isodate exception messages - #1122
mariocj89 merged 1 commit into
Conversation
Sorry, something went wrong.
mariocj89
left a comment
There was a problem hiding this comment.
Thanks a lot for the PR! It's almost there.
Sorry, something went wrong.
| if pos < len(datestr): | ||
| raise ValueError('String contains unknown ISO ' + | ||
| 'components: {}'.format(datestr)) | ||
| 'components: {}'.format(datestr.decode())) |
There was a problem hiding this comment.
Right, this is happening as the function uses _takes_ascii. Can you pass "ascii" as the codec to decode the bytes? In case the default encoding is another one.
Sorry, something went wrong.
| isoparser().parse_isodate(isostr) | ||
|
|
||
| # ensure the error message does not contain b' prefixes | ||
| assert "b'{}'".format(isostr) not in str(excinfo.value) |
There was a problem hiding this comment.
Rather than checking for the not presence of b'X', would you mind adding a new test case for this (feel free to copy this one) and validate the fully generated exception str?
Sorry, something went wrong.
|
@mariocj89 Thanks so much for the good feedback! I've added fixes for the things you've mentioned. Let me know if I need to make any additional tweaks. |
Sorry, something went wrong.
| if pos < len(datestr): | ||
| raise ValueError('String contains unknown ISO ' + | ||
| 'components: {}'.format(datestr)) | ||
| 'components: {}'.format(datestr.decode('ascii'))) |
There was a problem hiding this comment.
Nit: I think it'd be even easier to read if we passed here: 'components: {!r}'.
That will put ' around the text that failed (Say for example that we have some invisible characters.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed: f28e639
Definitely agree with removing the quotes making it more difficult to detect things like whitespace.
Sorry, something went wrong.
As the function is wrapped around a decorator that converts the input to bytes, when an exception is raised that includes the value, it contains the value as bytes (b"<val>"). To provide the expected str in Python 3, decode that to unicode. This will result in `u""` for Python 2 callers, but given that we plan to drop support for Python 2 in the short term, it feels acceptable.
|
Thanks, I've added a fixup for your tests to pass in Py2. I'll land this later today. |
Sorry, something went wrong.
Summary of changes
I started working on #383 by adding type hints to this branch: https://github.com/dateutil/dateutil/compare/master...pawl:monkeytype_merge_pyi?expand=1#diff-fba91b4117b31a0a29957adeb621fe79541fdb2424d10359e444df0d1cea2473R4
When I run mypy to see if those type hints work, I get a bunch of errors (a lot of which are probably my fault): https://gist.github.com/pawl/7ad1e9ce650b3241cc106d238d7edf04
This PR fixes one of those errors:
That error is saying isoparser().parse_isotime('2014-04-19T') will give an error that looks like this with the b' prefix around the input:
This is happening because the _takes_ascii decorator turns the string input into a bytestring, and python 3 represents bytestrings with b' when you pass them into .format().
This PR fixes the issue by using .decode() on the bytestring before using it with .format() for the error message.
Pull Request Checklist