Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add wait optional argument to hooks.editor #1657

Merged
merged 1 commit into from May 10, 2012
Merged

Add wait optional argument to hooks.editor #1657

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions IPython/core/hooks.py
View file
Open in desktop
Expand Up @@ -42,6 +42,7 @@ def calljed(self,filename, linenum):
#*****************************************************************************

import os, bisect
import subprocess
import sys

from IPython.core.error import TryNext
Expand All @@ -54,7 +55,7 @@ def calljed(self,filename, linenum):
'show_in_pager','pre_prompt_hook',
'pre_run_code_hook', 'clipboard_get']

def editor(self,filename, linenum=None):
def editor(self, filename, linenum=None, wait=True):
"""Open the default editor at the given filename and linenumber.

This is IPython's default editor hook, you can use it as an example to
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point! Let's play it safe and keep it as-is then.

if wait and proc.wait() != 0:
raise TryNext()

import tempfile
Expand Down