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.

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

Share code for magic_edit #460

Merged
merged 3 commits into from May 26, 2011
Merged

Share code for magic_edit #460

merged 3 commits into from May 26, 2011

Conversation

takluyver
Copy link
Member

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
Copy link
Member

minrk commented May 21, 2011

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
Copy link
Member Author

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
Copy link
Member

minrk commented May 21, 2011

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
Copy link
Member Author

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
Copy link
Member

minrk commented May 22, 2011

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member

fperez commented May 26, 2011

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 May 26, 2011
@fperez
Copy link
Member

fperez commented May 26, 2011

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants