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

Use str to get the exception message #3912

Merged
merged 3 commits into from Sep 6, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Apr 5, 2018

We are catching exceptions of type Exception, which no always have the message attribute (python3 at least).

e = Exception('Bu!')
assert not hasattr(e, 'message')
Copy link
Member

@humitos humitos left a comment

Nice!

We do have some custom exception where we define a message. Example:

https://github.com/rtfd/readthedocs.org/blob/9958feb644f638de6f8198f50cc0770ff42b980a/readthedocs/doc_builder/exceptions.py#L29

What would happen in those cases?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 6, 2018

@humitos if all the custom exceptions are calling their parent Exception with the message, everything is ok.

https://github.com/rtfd/readthedocs.org/blob/9958feb644f638de6f8198f50cc0770ff42b980a/readthedocs/doc_builder/exceptions.py#L13

@humitos
Copy link
Member

@humitos humitos commented Apr 6, 2018

Good.

Can you check that all our custom exception are like that?

Copy link
Member

@berkerpeksag berkerpeksag left a comment

Use of str() may cause problems especially in Python 2. Are we sure that none of these exception messages can contain non-ASCII characters?

Also, I'm probably missing something, but why did use builtins.str instead of just str?

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 16, 2018

@berkerpeksag I take the import from http://python-future.org/compatible_idioms.html#unicode

I check for an exception with non-ascii characters, this is what a found

>>> e = Exception('pitón')
>>> str(e)
'pit\xc3\xb3n'
>>> e.message
'pit\xc3\xb3n'
>>> from builtins import str
>>> e = Exception('pitón')
>>> str(e)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/site-packages/future/types/newstr.py", line 102, in __new__
    return super(newstr, cls).__new__(cls, value)
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 3: ordinal not in range(128)

With the imported str an exception happens. So, are we fine just removing the import?

@berkerpeksag
Copy link
Member

@berkerpeksag berkerpeksag commented Apr 16, 2018

Ah, so builtins is coming from the future module. I forgot that there is no builtins module in Python 2. It has been a while since I wrote code in Python 2 :)

I think using future and their implicit imports to do this is a bit bad idea. Even without using its implicit conversation, it's possible to get a UnicodeEncodeError:

>>> e = Exception(u'ıçğü')
>>> str(e)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-3: ordinal not in range(128)

(Since Django and other libraries use unicode to store data internally, the chance of getting a unicode exc.message (u'ı' for example) is more than a str ('ı' for example)

I'd be more conservative and don't do this until we get rid of Python 2, but of course this is just my opinion. If we decide to get this in, it would be nice to check if we reach 100% branch coverage (including adding tests for the u'non-ascıı' case) for both usages of str(exc).

@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 17, 2018

Maybe put this for python 3 milestone is the best

Copy link
Contributor

@agjohnson agjohnson left a comment

I'd prefer to see some tests here, it seems we're disregarding some of our use cases of exceptions here.

More specifically, exceptions that use message.

I agree on holding off here until this either has tests or we're closer to python 3.

@@ -5,6 +5,7 @@
absolute_import, division, print_function, unicode_literals)

import logging
from builtins import str
Copy link
Contributor

@agjohnson agjohnson Apr 17, 2018

builtins is third party import

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Apr 17, 2018

We're targeting 3.6 swap in prod in the move to new host. I'll target this at 2.5 milestone for now.

@agjohnson agjohnson added this to the 2.5 milestone Apr 17, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Apr 18, 2018

@agjohnson

it seems we're disregarding some of our use cases of exceptions here.
More specifically, exceptions that use message.

Are you referring to a code that uses e.message? All custom exceptions that I found pass the message attribute to their parent init method (till Exception), so e.message == str(e).

At least an exception change the e.message attribute after instantiating the object.

@stsewd stsewd force-pushed the str-on-exceptions branch from ae077e9 to 56ca14f Aug 22, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Aug 22, 2018

Rebased and updated, we could remove the builtins import when dropping the python2 compatibility #4543

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 6, 2018

Prod is 3.6 now, probably time to merge this!

@agjohnson agjohnson merged commit d704631 into readthedocs:master Sep 6, 2018
1 check passed
@stsewd stsewd deleted the str-on-exceptions branch Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants