Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Share code for magic_edit #460

Merged
merged 3 commits into from

4 participants

@takluyver
Owner

The core magic_edit and that for the zmqshell had a lot of duplicate code. The code in zmqshell hadn't been updated, leading to part of issue #426. This pulls the code to find the target of %edit out into a common helper function, _find_edit_target, which is called by both magic_edit functions.

Although this fixes the bug reported in #426, it's still not possible to use %edit in the Qt console. Hopefully, when the Qt console has config support (#175), it will be easy to set an editor for it.

@minrk
Owner

The qtconsole already has a configurable for editing, but it's just not useful yet, since it's not hooked up to configuration. As soon as that happens, it should be all set (setting the default value manually already works).

@takluyver
Owner

Yes, I checked that it worked by editing the module manually. It still doesn't behave quite like the in-process version, though - in particular, it doesn't seem to execute the edited code.

@minrk
Owner

No, it doesn't, but you generally can't rely on GUI apps notifying you that editing is complete, so I think that's intentional.

@takluyver
Owner

Hmm. We could just block until the editor closes, then execute the file. But I suspect that wouldn't work very well with an editor that can have multiple documents open, and that's most editors more advanced than notepad. But then %recall and %loadpy let you edit something at the Qt console before executing it.

Actually, the biggest annoyance would be macros - in the terminal, you can %edit a macro, and it updates the macro automatically. Without that, you'd have to redefine the macro, either from a string, or by %recall-ing it, editing it, running it, and grabbing it.

@minrk
Owner

Blocking until the editor closes is exactly what we do now, right? As you said, this often doesn't work as GUI apps (e.g. gedit) frequently return immediately if existing sessions do exist. That said, many do not. TextMate has a mate -w call that returns at document close, emacs seems to launch a new window each time, notepad on Windows behaves exactly as expected, etc. Googling revealed that even gedit has a --wait flag in the current development HEAD, but not in any release.

We rely on users to have a command that behaves as expected in the EDITOR env variable, I think it's okay to rely on similar knowledge when setting the GUI editor. It just means gedit is an invalid (or at least unsafe) choice for editor with a GUI IPython.

I think the qtconsole should probably not handle the edit magic in a special manner like it does now, but the zmqkernel should also not set the default editor from the EDITOR env variable.

That said, since the Qt frontend can be on a physically separate machine from its kernel, maybe the GUI launching the edit command does still make sense.

@epatters

The qtconsole must have the option of handling the edit magic in a special manner. I am thinking specifically of the use case where the widget is being embedded inside a larger application which has editing functionality. This use case is important to me, and may become important to others in the future.

@takluyver
Owner

I think it makes sense that the frontend deals with editing. But maybe this discussion should go onto the list. The pull request is a simple refactoring/bugfix, so that %edit works on the kernel side.

@fperez
Owner

Indeed, the discussion on the behavior of %edit in the various client contexts is slightly orthogonal to this PR. I've reviewed it and it looks good, also tested things both with the suite and interactively; all OK so far.

Thanks a lot for the cleanup, Thomas. Great work, as always.

I'll just merge it now.

@fperez fperez merged commit 47038ca into ipython:master
@fperez
Owner

And BTW, that discussion is important, and OK to have on-list or on a separate issue (with a link to what's been said so far for background). I just wanted to flush the PR as it was ready.

@damianavila damianavila referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@damianavila damianavila referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@damianavila damianavila referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 137 additions and 240 deletions.
  1. +131 −119 IPython/core/magic.py
  2. +6 −121 IPython/zmq/zmqshell.py
View
250 IPython/core/magic.py
@@ -89,6 +89,9 @@ def needs_local_scope(func):
"""Decorator to mark magic functions which need to local scope to run."""
func.needs_local_scope = True
return func
+
+# Used for exception handling in magic_edit
+class MacroToEdit(ValueError): pass
#***************************************************************************
# Main class implementing Magic functionality
@@ -2020,7 +2023,6 @@ def magic_macro(self,parameter_s = ''):
'print macro_name'.
"""
-
opts,args = self.parse_options(parameter_s,'r',mode='list')
if not args: # List existing macros
return sorted(k for k,v in self.shell.user_ns.iteritems() if\
@@ -2111,6 +2113,126 @@ def magic_loadpy(self, arg_s):
else:
content = open(arg_s).read()
self.set_next_input(content)
+
+ def _find_edit_target(self, args, opts, last_call):
+ """Utility method used by magic_edit to find what to edit."""
+
+ def make_filename(arg):
+ "Make a filename from the given args"
+ try:
+ filename = get_py_filename(arg)
+ except IOError:
+ # If it ends with .py but doesn't already exist, assume we want
+ # a new file.
+ if args.endswith('.py'):
+ filename = arg
+ else:
+ filename = None
+ return filename
+
+ # Set a few locals from the options for convenience:
+ opts_prev = 'p' in opts
+ opts_raw = 'r' in opts
+
+ # custom exceptions
+ class DataIsObject(Exception): pass
+
+ # Default line number value
+ lineno = opts.get('n',None)
+
+ if opts_prev:
+ args = '_%s' % last_call[0]
+ if not self.shell.user_ns.has_key(args):
+ args = last_call[1]
+
+ # use last_call to remember the state of the previous call, but don't
+ # let it be clobbered by successive '-p' calls.
+ try:
+ last_call[0] = self.shell.displayhook.prompt_count
+ if not opts_prev:
+ last_call[1] = parameter_s
+ except:
+ pass
+
+ # by default this is done with temp files, except when the given
+ # arg is a filename
+ use_temp = True
+
+ data = ''
+
+ # First, see if the arguments should be a filename.
+ filename = make_filename(args)
+ if filename:
+ use_temp = False
+ elif args:
+ # Mode where user specifies ranges of lines, like in %macro.
+ data = self.extract_input_lines(args, opts_raw)
+ if not data:
+ try:
+ # Load the parameter given as a variable. If not a string,
+ # process it as an object instead (below)
+
+ #print '*** args',args,'type',type(args) # dbg
+ data = eval(args, self.shell.user_ns)
+ if not isinstance(data, basestring):
+ raise DataIsObject
+
+ except (NameError,SyntaxError):
+ # given argument is not a variable, try as a filename
+ filename = make_filename(args)
+ if filename is None:
+ warn("Argument given (%s) can't be found as a variable "
+ "or as a filename." % args)
+ return
+ use_temp = False
+
+ except DataIsObject:
+ # macros have a special edit function
+ if isinstance(data, Macro):
+ raise MacroToEdit(data)
+
+ # For objects, try to edit the file where they are defined
+ try:
+ filename = inspect.getabsfile(data)
+ if 'fakemodule' in filename.lower() and inspect.isclass(data):
+ # class created by %edit? Try to find source
+ # by looking for method definitions instead, the
+ # __module__ in those classes is FakeModule.
+ attrs = [getattr(data, aname) for aname in dir(data)]
+ for attr in attrs:
+ if not inspect.ismethod(attr):
+ continue
+ filename = inspect.getabsfile(attr)
+ if filename and 'fakemodule' not in filename.lower():
+ # change the attribute to be the edit target instead
+ data = attr
+ break
+
+ datafile = 1
+ except TypeError:
+ filename = make_filename(args)
+ datafile = 1
+ warn('Could not find file where `%s` is defined.\n'
+ 'Opening a file named `%s`' % (args,filename))
+ # Now, make sure we can actually read the source (if it was in
+ # a temp file it's gone by now).
+ if datafile:
+ try:
+ if lineno is None:
+ lineno = inspect.getsourcelines(data)[1]
+ except IOError:
+ filename = make_filename(args)
+ if filename is None:
+ warn('The file `%s` where `%s` was defined cannot '
+ 'be read.' % (filename,data))
+ return
+ use_temp = False
+
+ if use_temp:
+ filename = self.shell.mktempfile(data)
+ print 'IPython will make a temporary file named:',filename
+
+ return filename, lineno, use_temp
def _edit_macro(self,mname,macro):
"""open an editor with the macro data in a file"""
@@ -2126,7 +2248,7 @@ def _edit_macro(self,mname,macro):
def magic_ed(self,parameter_s=''):
"""Alias to %edit."""
return self.magic_edit(parameter_s)
-
+
@skip_doctest
def magic_edit(self,parameter_s='',last_call=['','']):
"""Bring up an editor and execute the resulting code.
@@ -2269,122 +2391,13 @@ def magic_edit(self,parameter_s='',last_call=['','']):
starting example for further modifications. That file also has
general instructions on how to set a new hook for use once you've
defined it."""
-
- # FIXME: This function has become a convoluted mess. It needs a
- # ground-up rewrite with clean, simple logic.
-
- def make_filename(arg):
- "Make a filename from the given args"
- try:
- filename = get_py_filename(arg)
- except IOError:
- if args.endswith('.py'):
- filename = arg
- else:
- filename = None
- return filename
-
- # custom exceptions
- class DataIsObject(Exception): pass
-
opts,args = self.parse_options(parameter_s,'prxn:')
- # Set a few locals from the options for convenience:
- opts_prev = 'p' in opts
- opts_raw = 'r' in opts
- # Default line number value
- lineno = opts.get('n',None)
-
- if opts_prev:
- args = '_%s' % last_call[0]
- if not self.shell.user_ns.has_key(args):
- args = last_call[1]
-
- # use last_call to remember the state of the previous call, but don't
- # let it be clobbered by successive '-p' calls.
try:
- last_call[0] = self.shell.displayhook.prompt_count
- if not opts_prev:
- last_call[1] = parameter_s
- except:
- pass
-
- # by default this is done with temp files, except when the given
- # arg is a filename
- use_temp = True
-
- data = ''
- if args.endswith('.py'):
- filename = make_filename(args)
- use_temp = False
- elif args:
- # Mode where user specifies ranges of lines, like in %macro.
- data = self.extract_input_lines(args, opts_raw)
- if not data:
- try:
- # Load the parameter given as a variable. If not a string,
- # process it as an object instead (below)
-
- #print '*** args',args,'type',type(args) # dbg
- data = eval(args, self.shell.user_ns)
- if not isinstance(data, basestring):
- raise DataIsObject
-
- except (NameError,SyntaxError):
- # given argument is not a variable, try as a filename
- filename = make_filename(args)
- if filename is None:
- warn("Argument given (%s) can't be found as a variable "
- "or as a filename." % args)
- return
- use_temp = False
-
- except DataIsObject:
- # macros have a special edit function
- if isinstance(data, Macro):
- self._edit_macro(args,data)
- return
-
- # For objects, try to edit the file where they are defined
- try:
- filename = inspect.getabsfile(data)
- if 'fakemodule' in filename.lower() and inspect.isclass(data):
- # class created by %edit? Try to find source
- # by looking for method definitions instead, the
- # __module__ in those classes is FakeModule.
- attrs = [getattr(data, aname) for aname in dir(data)]
- for attr in attrs:
- if not inspect.ismethod(attr):
- continue
- filename = inspect.getabsfile(attr)
- if filename and 'fakemodule' not in filename.lower():
- # change the attribute to be the edit target instead
- data = attr
- break
-
- datafile = 1
- except TypeError:
- filename = make_filename(args)
- datafile = 1
- warn('Could not find file where `%s` is defined.\n'
- 'Opening a file named `%s`' % (args,filename))
- # Now, make sure we can actually read the source (if it was in
- # a temp file it's gone by now).
- if datafile:
- try:
- if lineno is None:
- lineno = inspect.getsourcelines(data)[1]
- except IOError:
- filename = make_filename(args)
- if filename is None:
- warn('The file `%s` where `%s` was defined cannot '
- 'be read.' % (filename,data))
- return
- use_temp = False
-
- if use_temp:
- filename = self.shell.mktempfile(data)
- print 'IPython will make a temporary file named:',filename
+ filename, lineno, is_temp = self._find_edit_target(args, opts, last_call)
+ except MacroToEdit as e:
+ self._edit_macro(args, e.args[0])
+ return
# do actual editing here
print 'Editing...',
@@ -2392,7 +2405,7 @@ class DataIsObject(Exception): pass
try:
# Quote filenames that may have spaces in them
if ' ' in filename:
- filename = "%s" % filename
+ filename = "'%s'" % filename
self.shell.hooks.editor(filename,lineno)
except TryNext:
warn('Could not open editor')
@@ -2407,15 +2420,14 @@ class DataIsObject(Exception): pass
print
else:
print 'done. Executing edited code...'
- if opts_raw:
+ if 'r' in opts: # Untranslated IPython code
self.shell.run_cell(file_read(filename),
store_history=False)
else:
self.shell.safe_execfile(filename,self.shell.user_ns,
self.shell.user_ns)
-
- if use_temp:
+ if is_temp:
try:
return open(filename).read()
except IOError,msg:
View
127 IPython/zmq/zmqshell.py
@@ -29,6 +29,7 @@
from IPython.core.displayhook import DisplayHook
from IPython.core.displaypub import DisplayPublisher
from IPython.core.macro import Macro
+from IPython.core.magic import MacroToEdit
from IPython.core.payloadpage import install_payload_page
from IPython.utils import io
from IPython.utils.path import get_py_filename
@@ -399,130 +400,14 @@ def magic_edit(self,parameter_s='',last_call=['','']):
general instructions on how to set a new hook for use once you've
defined it."""
- # FIXME: This function has become a convoluted mess. It needs a
- # ground-up rewrite with clean, simple logic.
-
- def make_filename(arg):
- "Make a filename from the given args"
- try:
- filename = get_py_filename(arg)
- except IOError:
- if args.endswith('.py'):
- filename = arg
- else:
- filename = None
- return filename
-
- # custom exceptions
- class DataIsObject(Exception): pass
-
opts,args = self.parse_options(parameter_s,'prn:')
- # Set a few locals from the options for convenience:
- opts_p = opts.has_key('p')
- opts_r = opts.has_key('r')
- # Default line number value
- lineno = opts.get('n',None)
- if lineno is not None:
- try:
- lineno = int(lineno)
- except:
- warn("The -n argument must be an integer.")
- return
-
- if opts_p:
- args = '_%s' % last_call[0]
- if not self.shell.user_ns.has_key(args):
- args = last_call[1]
-
- # use last_call to remember the state of the previous call, but don't
- # let it be clobbered by successive '-p' calls.
try:
- last_call[0] = self.shell.displayhook.prompt_count
- if not opts_p:
- last_call[1] = parameter_s
- except:
- pass
-
- # by default this is done with temp files, except when the given
- # arg is a filename
- use_temp = True
-
- data = ''
- if args[0].isdigit():
- # Mode where user specifies ranges of lines, like in %macro.
- # This means that you can't edit files whose names begin with
- # numbers this way. Tough.
- ranges = args.split()
- data = ''.join(self.extract_input_slices(ranges,opts_r))
- elif args.endswith('.py'):
- filename = make_filename(args)
- use_temp = False
- elif args:
- try:
- # Load the parameter given as a variable. If not a string,
- # process it as an object instead (below)
-
- #print '*** args',args,'type',type(args) # dbg
- data = eval(args, self.shell.user_ns)
- if not isinstance(data, basestring):
- raise DataIsObject
-
- except (NameError,SyntaxError):
- # given argument is not a variable, try as a filename
- filename = make_filename(args)
- if filename is None:
- warn("Argument given (%s) can't be found as a variable "
- "or as a filename." % args)
- return
- use_temp = False
-
- except DataIsObject:
- # macros have a special edit function
- if isinstance(data, Macro):
- self._edit_macro(args,data)
- return
-
- # For objects, try to edit the file where they are defined
- try:
- filename = inspect.getabsfile(data)
- if 'fakemodule' in filename.lower() and inspect.isclass(data):
- # class created by %edit? Try to find source
- # by looking for method definitions instead, the
- # __module__ in those classes is FakeModule.
- attrs = [getattr(data, aname) for aname in dir(data)]
- for attr in attrs:
- if not inspect.ismethod(attr):
- continue
- filename = inspect.getabsfile(attr)
- if filename and 'fakemodule' not in filename.lower():
- # change the attribute to be the edit target instead
- data = attr
- break
-
- datafile = 1
- except TypeError:
- filename = make_filename(args)
- datafile = 1
- warn('Could not find file where `%s` is defined.\n'
- 'Opening a file named `%s`' % (args,filename))
- # Now, make sure we can actually read the source (if it was in
- # a temp file it's gone by now).
- if datafile:
- try:
- if lineno is None:
- lineno = inspect.getsourcelines(data)[1]
- except IOError:
- filename = make_filename(args)
- if filename is None:
- warn('The file `%s` where `%s` was defined cannot '
- 'be read.' % (filename,data))
- return
- use_temp = False
-
- if use_temp:
- filename = self.shell.mktempfile(data)
- print('IPython will make a temporary file named:', filename)
+ filename, lineno, _ = self._find_edit_target(args, opts, last_call)
+ except MacroToEdit as e:
+ # TODO: Implement macro editing over 2 processes.
+ print("Macro editing not yet implemented in 2-process model.")
+ return
# Make sure we send to the client an absolute path, in case the working
# directory of client and kernel don't match
Something went wrong with that request. Please try again.