Preserve cause of exception when getting/setting attribute in DeviceProxy #365
Conversation
|
I guess we still need to support python 2? from six import raise_from |
Yes, please! |
| if cause is None: | ||
| cause = e |
There was a problem hiding this comment.
There would be multiple exceptions in this method if the device is off/unreachable, so keeping the first exception is a good approach 👍
tango/device_proxy.py
Outdated
| return super(DeviceProxy, self).__setattr__(name, value) | ||
| except Exception as e: | ||
| raise e from cause |
There was a problem hiding this comment.
This case is interesting. If we get here then name isn't a known Tango device command/attribute/pipe. We end up writing to either an existing Python class instance attribute, or adding a new one, if it doesn't exist yet.
I'm not sure that __setattr__ can even raise an exception? If it did, the cause may or may not be relevant.
A couple of cases could occur for the example of an unreachable device:
- We haven't yet cached the lists of commands, attributes and pipes.
- We have cached the lists of commands, attributes and pipes prior to this call.
Then we get these four scenarios, depending on if the Tango command/attribute/pipe either:
- Exists on the device:
- For case 1: we'll write to a local class instance attribute and the user won't even get an error. This behaviour is unexpected, but not a new problem.
- For case 2: we would have failed because it is a command, or we tried to call
__set_attribute_valueorwrite_pipeearlier in this method, so the user will get whatever error those methods throw. That's fine.
- Doesn't exist on the device:
- For case 1: we'll write to a local class instance attribute and the user won't get an error. This could be the desired behaviour, if the user was storing some other info on an instance of a
DeviceProxy. Or, it could be undesired, if the user thought they were setting a Tango attribute/pipe but had the wrong name. Not much we can do about that. - For case 2: same as case 1. There won't be an error, and we're not sure if the user expected one of not.
- For case 1: we'll write to a local class instance attribute and the user won't get an error. This could be the desired behaviour, if the user was storing some other info on an instance of a
In conclusion, the change in the PR is fine. It has just made us aware of some nuances in how setting attributes on a DeviceProxy behaves.
For some reason the CI tests for Python 2.7 are attempting to install a version of pytest-xdist that no longer supports Python 2.7. Explicitly limit the version to avoid this.
tango/device_proxy.py
Outdated
| pass | ||
| except Exception as e: | ||
| if cause is None: | ||
| cause = e | ||
|
|
||
| if name_l in self.__get_cmd_cache(): | ||
| raise TypeError('Cannot set the value of a command') |
There was a problem hiding this comment.
Shouldn't this line include cause, similar to line 332?
There was a problem hiding this comment.
Good catch. Thanks!
ajoubertza
changed the title
Closes #364