ISISComputingGroup / ISISComputingGroup/lewis
Standardize Exception Handling
- Dominant language
- Python
- Stars
- 24
- Forks
- 19
- PR merge metrics
- No merged PRs in 30d
Description
Currently, exception handling is very ad hoc throughout the system.
The following exception classes exist:
- `StateMachineException` (statemachine.py)
- `RemoteException` (control_client.py)
- `ProtocolException` (control_client.py)
The latter two are probably fine as-is, since the control client is arguably its own application/library and those two exception types work well to capture and differentiate what can go wrong during its usage.
The simulation itself, however, needs better exception types and usage practices. Currently, only the StateMachineException exists as a distinct exception type. Any time an exception needs to be raised outside of that, standard classes like RuntimeError are used. _Even inside the StateMachine, sometimes a RuntimeError is raised instead of StateMachineException._
This makes catching and handling exceptions difficult and, consequently, we don't do that very well either.
Ideas to consider/discuss:
- We should keep in mind that there are two distinct usage scenarios for the simulation: Command-line and as an imported library.
- Should we have a `core/exceptions.py` or similar to define all exceptions used throughout the system?
- Should we avoid allowing anything apart from exceptions we define to escape our code as a matter of principle? Raising standard exceptions within the system would be fine as long as they are handled before reaching the user under this model.
- One could argue that exceptions like AttributeError, ValueError, NotImplementedError, etc are descriptive enough and don't need to be wrapped in our own type / would be better as they are.
- Any of the above exceptions are meaningless if they come from too deep in our code. Getting an `IndexError` makes sense if you as the user provided a bad index. It doesn't make sense if you called a function that doesn't even take parameters, but something went wrong deep inside the core code (like, say, `pcaspy` couldn't be imported).
- RuntimeError, however, is so generic that I don't think it should ever escape our code (or perhaps even ever be raised?).
- On the other hand, we could consider not using our own exception types at all (including getting rid of StateMachineException) and just using standard exceptions for everything (in which case RuntimeError might be fine if there isn't anything more appropriate). This might make handling exceptions awkward though.
- When run from the command-line, we should have some high-level exception handling to make sure no exceptions escape to become traces. Errors should be displayed in a human readable way for the user, and ideally be specific enough to enable finding a solution easily.
- Interrupts like CTRL-C should also be handled gracefully.
This issue can be used to discuss how to best solve this.
This issue should probably be addressed after dealing with #125, #132 and #133.
Contributor guide
Assessment
This issue has not been assessed yet.