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
fix quarantine/ipy_editors.py #2640
Conversation
…instead of some old old 'itpl' module (1) doesn't use the deprecated ipyapi, (2) fully pep8 compliant, (3) better docstring
# We no longer bundle Itpl. If you update this module, you should use advanced | ||
# string formatting instead. | ||
from IPython.external.Itpl import itplns | ||
from string import Template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, I'd be inclined to use str.format()
with {file}
style fields - I think people are more familiar with that than with string.Template
. But that's open to discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure that's fine. I just used template because it required changing less code :)
You're probably right though. (I'd never heard of string.Template until I looked it up just for this)
Thanks. If it's going to be useful to people, though, it will need to come out of quarantine, because those modules aren't installed by default. We could remove some of the less popular editors for now (I've never even heard of Crimson edit or GNUclient, for example). Since it doesn't do anything without user intervention, it can't cause a regression, so I think it's OK not to have tested with every editor on the list. I'd call the module |
Oooh. didn't realize about the quarantine not actually being installed, since I was using Yeah, I've never heard of the those editors either. Sounds good to me. |
Stupid question, the edit magic is different for qtconsole right? Thanks ! |
I think so?
Either this script should make this clear, or perhaps it should be able to hook into this system. After all, most of the editors that it references are gui apps. |
Good point, @Carreau . Yes, ideally I'd like to see this support the Qt console as well. In practice, I don't know how easy that is, because of the frontend/kernel separation. As I understand it, when you call %edit in the Qt console, the contents of the file are transmitted to the frontend for it to open an editor. Obviously that's useful if your kernel isn't on the same machine as the frontend, but it means it's tricky for anything in the kernel to affect the editor. |
So, do we move this out of quarantine ? |
I will just improve the PR and fix it for the qtconsole. |
On second though, after banging my head against this for about an hour, I don't think I understand the qtconsole code well enough to do this. Looking at the code on like 475 of How do you get access to the active |
There is some Travis issue:
|
You can't access that object from the kernel, because it's in the frontend. It is configurable, though - see the definitions at line 62 |
My solution to getting run-time configurable %edit in the qtconsole is a bit of a hack, but it works (I tested emacs and textmate). Basically, I modified
It seems a bit hacky to me. The problem is basically that the implementation in the TerminalInteractiveShell with the hooks and stuff is totally different than the qtconsole. For instance, |
I think we need to have a more careful work through the code that implements %edit over ZMQ - at the moment, it's rather complex because there's an entirely separate configuration system for editing in the frontend. In the meantime, I'd be inclined to merge this on the grounds that we shouldn't let aiming for perfection delay improvement. I'm a bit concerned about changing the messaging format to add So, while I realise that it's going back on what I said before, I'm now inclined to leave the ZMQ-related changes out, and merge the rest of this. Any thoughts? |
Let's not implement the %edit magic in a hacky way over zeromq. We have plans to do it right and should wait for that. |
@@ -321,10 +321,16 @@ def edit(self, parameter_s='', last_call=['','']): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's back out the changes to this file.
Okay. I agree with you guys that it's best not to try to do the %edit magic over zmq unless its going to be done "right", which this probably wasn't. Those changes have been backed out. |
""" | ||
|
||
from IPython.core.error import TryNext | ||
from IPython.frontend.qt.console import ipython_widget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed now? It's causing problems for the test suite.
I'd prefer if we could remove the requirement that the filename be in quotation marks. For example, just allow:
This could be accomplished by first passing the filename through
Since we are already changing the syntax, it'd be good to add this feature now. |
Good idea, @bfroehle. Done. |
Reviving old thread. This seem to have only upsides, shoudl we merge it and take care of the remaining point (edit over zmq) later? My vote : +1 for merging. |
This makes sense to me - if @bfroehle agrees, I say merge. |
Looks fine to me, but perhaps there should be some help as to how to use this... For example, I guess that I'm supposed to run something like:
Also it seems to be impossible to override your first setting because |
Yes, that's a bit of a weirdness about hooks in general. I think we can go ahead and merge this, then treat the fact that hooks cannot be replaced as a separate issue. |
fix quarantine/ipy_editors.py
Merged! :) |
fix quarantine/ipy_editors.py
I wanted to use textmate with
%ed
, so I found this module. It needed a little work, but not too much.fixes:
itpl
moduleper @ellisonbg's commit three years ago that moved it into quarantine, I don't think this module should be actually removed from quarantine:
Since I've never used many of the editors -- and the code depends on the format in which they like their arguments when invoked from the shell -- I can't confirm that all the code works. (It does work for emacs, textmate and idle on osx). Also, it's not really clear how to test this module, since you would need to have all the editors installed.
But regardless, the current
ipy_editors.py
assuredly does not work, and the code in this PR works at least for some editors (on some platforms). So it's an improvement, no?