Add wait optional argument to hooks.editor
#1657
Conversation
subprocess.Popen is used instead of os.system in order to spawn editor process in non-blocking manner.
|
Thanks for creating this; as I mentioned in #1655 I'm a bit swamped, will get back to all this in a few days. |
| @@ -76,7 +77,9 @@ def editor(self,filename, linenum=None): | |||
| editor = '"%s"' % editor | |||
|
|
|||
| # Call the actual editor | |||
| if os.system('%s %s %s' % (editor,linemark,filename)) != 0: | |||
| proc = subprocess.Popen('%s %s %s' % (editor, linemark, filename), | |||
| shell=True) | |||
There was a problem hiding this comment.
Actually, now that it's using subprocess, it might be better to make the call with shell=False (i.e. the default). We only used os.system because IPython dates back to before subprocess even existed. But using shell=True is slower (especially on Windows), and in this case we don't really need it, so we might as well update the code to current best practices.
There was a problem hiding this comment.
I wanted to avoid shell=True but I thought IPython allows space in editor config, no? For example, emacsclient -tty. If user want to set a path to editor program containing spaces (especially on Windows), it will be problem because you can't do editor.split(). You can use shlex in stdlib but I think it's too much for this purpose. What do you think?
There was a problem hiding this comment.
Ah, good point! Let's play it safe and keep it as-is then.
|
Looks good, merging now. Thanks! |
Add `wait` optional argument to `hooks.editor`. As indicated the discussion for #1655 (superseded by this PR), this allows users to define custom magics that can call editors without waiting for a return value.
Add `wait` optional argument to `hooks.editor`. As indicated the discussion for ipython#1655 (superseded by this PR), this allows users to define custom magics that can call editors without waiting for a return value.
subprocess.Popen is used instead of os.system in order to spawn editor process in non-blocking manner.
See also: #1655
I think there are two ways to improve this patch (as @takluyver suggested):
wait == False, sleep short time (0.1 sec) right after starting editor process to check if the editor exits with non-zero exit code.wait == Falseso that message will not be printed in unexpected way (like after the nextIn[]prompt). If we add sleep, error message can be printed only when editor exits immediately with an error.Please tell me if you want to implement these.