Forward MatlabInterperterError to the user #118
Conversation
…ke sure it gets raised instead of generic RuntimeError so the user gets some feedback. Fixes arokem#117.
|
Looks good. There seemed to be a lot of opportunity for cleanup in the surrounding code so I followed it up with another commit. Let me know if you run into issues! |
|
No problems here. |
| @@ -211,6 +211,8 @@ def matlab(self, line, cell=None, local_ns=None): | |||
| else: | |||
| e_s = "There was an error running the code:\n %s"%code | |||
| result_dict = self.eval(code) | |||
| except MatlabInterperterError: | |||
| raise | |||
| except: | |||
There was a problem hiding this comment.
What would happen with this second exception? Should we remove this one?
There was a problem hiding this comment.
I don't know enough about your code to say. I'm guessing that some other kind of exception is raised when a MATLAB session cannot be found and it would be caught here.
However, I agree that it should be removed if possible, it's not great that it may catch and hide other exceptions at this point to. Do you know what exception gets raised when the interpreter can't be found? If there isn't one, maybe one can be added and this problem could be avoided.
There was a problem hiding this comment.
We came across this before in #79, and we couldn't figure out what exception was meant to go there. It might make sense to just capture all exceptions from zmq (e.g. wrapped in MatlabCommunicationError or something like that) and catch just those here.
There was a problem hiding this comment.
Oh, and we should also wrap OSErrors from subprocess.Popen as well. That's the part that spawns the matlab process.
|
+1 @isbadawi |
This PR is to address #117. It ensures that MatlabInterperterError is re-raised so the user can get some feedback as to what caused the error if there was one. If anyone wants to improve on it, feel free.