reenable multiline history for terminals #838
Conversation
| hlen = self.readline.get_current_history_length() | ||
| lines = len(source_raw.splitlines()) | ||
| for i in range(1, min(hlen, lines) + 1): | ||
| self.readline.remove_history_item(hlen - i) |
There was a problem hiding this comment.
I assume you've tested this, but surely if you remove item 41 (for instance), the item that was previously 42 is now 41? So shouldn't we just remove an item from the same slot n times?
There was a problem hiding this comment.
you always remove from the end of the history so in effect its the same
There was a problem hiding this comment.
Oh I see. I'd overlooked that it was counting backwards.
|
please don't merge yet I noticed an issue that it deletes history when you recall history and execute it again as the its one line in history but it removes linecount items. This can be solved by counting the times it accepts more and removing that amount of items. |
|
Perhaps in the interact loop, you could keep a record of the readline history length at the start of the current cell, then remove any extra entries after that. |
|
Could I suggest clarifying variable names a bit, for people who read this code in the future. In particular, hlen (hlen_before_cell?), and _store_multiline_history (_replace_rlhist_multiline?). |
|
Is this something that can be easily tested? |
|
I think we could test calling the function ( |
| @@ -307,6 +307,9 @@ class InteractiveShell(SingletonConfigurable, Magic): | |||
| Automatically call the pdb debugger after every exception. | |||
| """ | |||
| ) | |||
| multiline_history = CBool(False, config=True, | |||
| help="Store multiple line spanning cells as a single entry in history." | |||
There was a problem hiding this comment.
This description could probably be clearer. We always store multiline cells in our own history; this affects what happens in readline history - the history you get by pressing up & down arrows, or using Ctrl-R.
Also, I think the config option should be on the TerminalInteractiveShell subclass (IPython.frontends.terminal.interactiveshell), since it doesn't affect other frontends.
There was a problem hiding this comment.
it needs to be in core as there the history is recalled from disk and one must decide to do it line by line or per cell
There was a problem hiding this comment.
The code that's initialising readline should now only be run by the terminal frontend (as self.readline_use is now False for the ZMQShell).
In time, we should move the init_readline code to TerminalInteractiveShell as well. In the meantime, just to be on the safe side, we can use getattr(self, 'multiline_history', True), which will behave even if the code does somehow get run by the other frontends.
There was a problem hiding this comment.
Instead of using getattr, let's make multiline_history a non-configurable trait on core.InteractiveShell, with a note that it should be removed when we finish getting the readline code out of the core.
There was a problem hiding this comment.
OK, that makes sense. Presumably then we override it with a configurable trait in TerminalInteractiveShell.
There was a problem hiding this comment.
@juliantaylor, does that make sense? It seems that this is almost ready to go, so if you implement @mink's suggestion and make the default true (as there seems to be interest in that), this should be in good shape.
I would, however, like to see a test added. A simple test that pushes two lines of history and gets them back intact should suffice. It must be protected with a decorator so it only runs when readline is present, though.
That will ensure we don't regress on this feature again in the future...
With those changes and a test, I don't see what else would be needed and we can proceed to merge. Thanks!
|
@fperez: Since at least some people feel strongly about this, I think it's worth getting into 0.12. I don't think that's likely to be a problem - it seems to be mostly ready now, it just needs a bit of tweaking and testing. Everyone: An important question is what the default should be. We changed to readline working line-by-line for 0.11, but a number of people have voiced a preference for the older joined-cell behaviour (see #571). I think I marginally prefer the joined-cell alternative. |
|
It can certainly go into 0.12, I'd be happy to see that. Note that it currently doesn't merge, so unfortunately it's in need of a rebase. @juliantaylor, let us know if you need a hand with that. @takluyver, as far as the default is concerned, I'm OK if we revert to joined-cell: I honestly think that the readline console is just the wrong environment for multiline work, so I don't really use it that way all that much myself. Hence I don't care too much, and I'm happy to go along with whatever people feel suits them best. |
Add configuration variable InteractiveShell.multiline_history. If it is True cells spanning multiple lines will be saved in history as a single entry instead of one entry per line, as was the case in ipython < 0.11. closes ipythongh-571
Remember previous history length to remove the correct amount of single line entries
|
OK, we don't want this to linger forever, and @takluyver made a good point on IRC: multiline on is really the better default b/c recovering a block that's hidden 20 lines back is a pain otherwise. I'm making the default be true and merging. Thanks! |
|
sorry, for not reacting for a while, I had some other priorities the last few days. |
mutliline PR #838 added a call to readline.add_history, but without the unicode/str protection used elsewhere, allowing `In[1]: u'é'` to crash IPython. This makes the call match others in core.interativeshell.
mutliline PR ipython#838 added a call to readline.add_history, but without the unicode/str protection used elsewhere, allowing `In[1]: u'é'` to crash IPython. This makes the call match others in core.interativeshell.
Add configuration variable InteractiveShell.multiline_history.
If it is True cells spanning multiple lines will be saved in history
as a single entry instead of one entry per line, as was the case in
ipython < 0.11.
closes gh-571