Skip to content

Throw friendly errors #342

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.

Sign up for GitHub

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

Merged
dwalton76 merged 4 commits into from
Sep 10, 2017
Merged

Throw friendly errors #342

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e26c36
Untested implementation of friendly errors
WasabiFan Jul 23, 2017
9efa543
Fix syntax
WasabiFan Aug 19, 2017
b009e9e
Use specific exception type
WasabiFan Aug 20, 2017
a0f77d4
Fix and test error implementation
WasabiFan Sep 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions ev3dev/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import shlex
import stat
import time
import errno
from os.path import abspath
from struct import pack, unpack
from subprocess import Popen, check_output, PIPE
Expand Down Expand Up @@ -204,12 +205,31 @@ def _set_attribute(self, attribute, name, value):
attribute = self._attribute_file_open( name )
else:
attribute.seek(0)
attribute.write(value.encode())
attribute.flush()

try:
attribute.write(value.encode())
attribute.flush()
except Exception as ex:
self._raise_friendly_access_error(ex, name)
return attribute
else:
raise Exception('Device is not connected')

def _raise_friendly_access_error(self, driver_error, attribute):
if not isinstance(driver_error, OSError):
raise driver_error

if driver_error.errno == errno.EINVAL:
if attribute == "speed_sp":
try:
max_speed = self.max_speed
except (AttributeError, Exception):
raise ValueError("The given speed value was out of range") from driver_error
else:
raise ValueError("The given speed value was out of range. Max speed: +/-" + str(max_speed)) from driver_error
raise ValueError("One or more arguments were out of range or invalid") from driver_error
Copy link
Copy Markdown
Member

@ddemidov ddemidov Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not convinced a value error is more friendly than OSError to be fair. OSError is the thing that happens in the background, why do we want to 'protect' the user from this?

May be it would be better to at least re-raise the original (OSError) exception instead of generic 'One or more arguments were out of range or invalid' for unsupported attributes?

Copy link
Copy Markdown
Member

@ddemidov ddemidov Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, what would make the debugging easier?

Copy link
Copy Markdown
Member Author

@WasabiFan WasabiFan Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not convinced a value error is more friendly than OSError to be fair. OSError is the thing that happens in the background, why do we want to 'protect' the user from this?

That the nice Python functions happen to be calling an operating system routine is an implementation detail. As far as the user is concerned, they called the function and provided an argument that was out-of-range; a ValueError is the Python concept that describes such an issue, and the message explains what went wrong. If the user is interested in the OSError, they can see it as the __cause__ of the ValueError (hence raise ... from).

For most casual programmers, the message OSError: [Errno 22] Invalid argument will be confusing -- it isn't natural to expect an OSError for this sort of scenario, and the message certainly isn't clear. Unless I'm directly consuming OS functions, I would generally expect an OSError to indicate an issue with the implementation of a library, not my own mistake.

May be it would be better to at least re-raise the original (OSError) exception instead of generic 'One or more arguments were out of range or invalid' for unsupported attributes?

See above; the inner exception is available, just within the user-friendly error that we throw.

Copy link
Copy Markdown
Member Author

@WasabiFan WasabiFan Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, what would make the debugging easier?

The error tells you exactly what you did wrong; no question is left as to what was going on when the error occurred or what caused it.

Copy link
Copy Markdown
Collaborator

@dwalton76 dwalton76 Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would make the debugging easier for me if the exception text always included

  • the name of the attribute we are trying to set
  • the value we are trying to set it to
  • the min/max or acceptable inputs for that attribute

that would be very useful

raise driver_error

def get_attr_int(self, attribute, name):
attribute, value = self._get_attribute(attribute, name)
return attribute, int(value)
Expand Down