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

Projects
None yet
4 participants
@stsewd
Member

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')
@humitos

Nice!

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

What would happen in those cases?

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Apr 6, 2018

Member

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

super(BuildEnvironmentException, self).__init__(message, **kwargs)

Member

stsewd commented Apr 6, 2018

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

super(BuildEnvironmentException, self).__init__(message, **kwargs)

@humitos

This comment has been minimized.

Show comment
Hide comment
@humitos

humitos Apr 6, 2018

Member

Good.

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

Member

humitos commented Apr 6, 2018

Good.

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

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Apr 6, 2018

Member

These class don't override the init method, so everything is ok

class LockTimeout(Exception):
pass

class NoProjectException(Exception):
pass

class ProjectSpamError(Exception):
"""
Error raised when a project field has detected spam.
This error is not raised to users, we use this for banning users in the
background.
"""
pass

class NoProjectException(Exception):
pass

These override the init method correctly

class TaskNotFound(Exception):
def __init__(self, task_id, *args, **kwargs):
message = 'No public task found with id {id}'.format(id=task_id)
super(TaskNotFound, self).__init__(message, *args, **kwargs)

class TaskNoPermission(Exception):
def __init__(self, task_id, *args, **kwargs):
message = 'No permission to access task with id {id}'.format(
id=task_id)
super(TaskNoPermission, self).__init__(message, *args, **kwargs)

I found those grepping the project.

Member

stsewd commented Apr 6, 2018

These class don't override the init method, so everything is ok

class LockTimeout(Exception):
pass

class NoProjectException(Exception):
pass

class ProjectSpamError(Exception):
"""
Error raised when a project field has detected spam.
This error is not raised to users, we use this for banning users in the
background.
"""
pass

class NoProjectException(Exception):
pass

These override the init method correctly

class TaskNotFound(Exception):
def __init__(self, task_id, *args, **kwargs):
message = 'No public task found with id {id}'.format(id=task_id)
super(TaskNotFound, self).__init__(message, *args, **kwargs)

class TaskNoPermission(Exception):
def __init__(self, task_id, *args, **kwargs):
message = 'No permission to access task with id {id}'.format(
id=task_id)
super(TaskNoPermission, self).__init__(message, *args, **kwargs)

I found those grepping the project.

@berkerpeksag

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Apr 16, 2018

Member

@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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Apr 16, 2018

Member

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).

Member

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Apr 17, 2018

Member

Maybe put this for python 3 milestone is the best

Member

stsewd commented Apr 17, 2018

Maybe put this for python 3 milestone is the best

@agjohnson

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.

Show outdated Hide outdated readthedocs/restapi/views/model_views.py Outdated
@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Apr 17, 2018

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Apr 18, 2018

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 22, 2018

Member

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

Member

stsewd commented Aug 22, 2018

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

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Sep 6, 2018

Contributor

Prod is 3.6 now, probably time to merge this!

Contributor

agjohnson commented Sep 6, 2018

Prod is 3.6 now, probably time to merge this!

@agjohnson agjohnson merged commit d704631 into rtfd:master Sep 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd: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