JeffLIrion / adb_shell

A Python implementation of ADB with shell and FileSync functionality.
Apache License 2.0
530 stars 60 forks source link

fix: Ensure `UsbReadFailedError` can be pickled #183

Closed maybe-sybr closed 3 years ago

maybe-sybr commented 3 years ago

This change ensures that the base Exception class is aware of the usb_error arg which gets passed to the subclass, rather than intercepting it. By doing so, we leverage the built-in __reduce__() behaviour of Exception so that UsbReadFailedError can be pickled and unpickled without blowing up.

This change causes a minor change to the representation of these exceptions, However stringification remains unchanged. Previously:

>>> e = adb_shell.exceptions.UsbReadFailedError("foo", usb1.USBError("bar"))
>>> str(e)
'foo: Unknown error [bar]'
>>> repr(e)
"UsbReadFailedError('foo')"

and with this change:

>>> e = adb_shell.exceptions.UsbReadFailedError("foo", usb1.USBError("bar"))
>>> str(e)
'foo: Unknown error [bar]'
>>> repr(e)
"UsbReadFailedError('foo', USBErrorTimeout())"
maybe-sybr commented 3 years ago

I came across some breakage while passing this exception across a worker boundary and landed on this being the fix. I can add a test if that's useful.

JeffLIrion commented 3 years ago

That would be great if you could add a test. How about a new file: test_exceptions.py?

maybe-sybr commented 3 years ago

That would be great if you could add a test. How about a new file: test_exceptions.py?

Sure thing, I'll get onto that shortly and push an amended diff.

maybe-sybr commented 3 years ago

I'm kind of disgusted with what I did in that test code... - let me know if it's too obtuse. I was aiming do a poor imitation of pytest.mark.parameterize() over all exceptions defined in adb_shell.exceptions module.

Edit: At least it should work under both Python 2.7 and 3.x now.

JeffLIrion commented 3 years ago

I'm kind of disgusted with what I did in that test code... - let me know if it's too obtuse. I was aiming do a poor imitation of pytest.mark.parameterize() over all exceptions defined in adb_shell.exceptions module.

Edit: At least it should work under both Python 2.7 and 3.x now.

That code is complicated. But the tests pass, so I'm OK with it.

I was thinking a test like your example code above:

>>> e = adb_shell.exceptions.UsbReadFailedError("foo", usb1.USBError("bar"))
>>> str(e)
'foo: Unknown error [bar]'
>>> repr(e)
"UsbReadFailedError('foo', USBErrorTimeout())"

And then assert the results of str(e) and repr(e).

maybe-sybr commented 3 years ago

I was thinking a test like your example code above:

>>> e = adb_shell.exceptions.UsbReadFailedError("foo", usb1.USBError("bar"))
>>> str(e)
'foo: Unknown error [bar]'
>>> repr(e)
"UsbReadFailedError('foo', USBErrorTimeout())"

And then assert the results of str(e) and repr(e).

Done in the new diff. The py2/3 compatible assertRegex is a bit clunky again but I didn't spot any current use of six/future so I just did it myself rather than introduce a dependency.

JeffLIrion commented 3 years ago

Thanks!