reenable multiline history for terminals #838

Merged
merged 3 commits into from Oct 18, 2011

Conversation

Projects
None yet
4 participants
@juliantaylor
Contributor

juliantaylor commented Oct 5, 2011

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

@takluyver

View changes

IPython/frontend/terminal/interactiveshell.py
+ 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)

This comment has been minimized.

@takluyver

takluyver Oct 5, 2011

Member

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?

@takluyver

takluyver Oct 5, 2011

Member

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?

This comment has been minimized.

@juliantaylor

juliantaylor Oct 5, 2011

Contributor

you always remove from the end of the history so in effect its the same

@juliantaylor

juliantaylor Oct 5, 2011

Contributor

you always remove from the end of the history so in effect its the same

This comment has been minimized.

@takluyver

takluyver Oct 5, 2011

Member

Oh I see. I'd overlooked that it was counting backwards.

@takluyver

takluyver Oct 5, 2011

Member

Oh I see. I'd overlooked that it was counting backwards.

@juliantaylor

This comment has been minimized.

Show comment
Hide comment
@juliantaylor

juliantaylor Oct 5, 2011

Contributor

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.
I'll test that a bit and reopen.

Contributor

juliantaylor commented Oct 5, 2011

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.
I'll test that a bit and reopen.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Oct 5, 2011

Member

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.

Member

takluyver commented Oct 5, 2011

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.

@juliantaylor juliantaylor reopened this Oct 6, 2011

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Oct 7, 2011

Member

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?).

Member

takluyver commented Oct 7, 2011

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?).

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 14, 2011

Member

Is this something that can be easily tested?

Member

minrk commented Oct 14, 2011

Is this something that can be easily tested?

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Oct 14, 2011

Member

I think we could test calling the function (_replace_rlhist_multiline) and checking the modifications it makes via readline.get_history_item. Obviously the actual user interaction is a bit trickier to test.

Member

takluyver commented Oct 14, 2011

I think we could test calling the function (_replace_rlhist_multiline) and checking the modifications it makes via readline.get_history_item. Obviously the actual user interaction is a bit trickier to test.

@@ -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."

This comment has been minimized.

@takluyver

takluyver Oct 15, 2011

Member

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.

@takluyver

takluyver Oct 15, 2011

Member

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.

This comment has been minimized.

@juliantaylor

juliantaylor Oct 16, 2011

Contributor

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

@juliantaylor

juliantaylor Oct 16, 2011

Contributor

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

This comment has been minimized.

@takluyver

takluyver Oct 16, 2011

Member

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.

@takluyver

takluyver Oct 16, 2011

Member

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.

This comment has been minimized.

@minrk

minrk Oct 16, 2011

Member

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.

@minrk

minrk Oct 16, 2011

Member

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.

This comment has been minimized.

@takluyver

takluyver Oct 16, 2011

Member

OK, that makes sense. Presumably then we override it with a configurable trait in TerminalInteractiveShell.

@takluyver

takluyver Oct 16, 2011

Member

OK, that makes sense. Presumably then we override it with a configurable trait in TerminalInteractiveShell.

This comment has been minimized.

@fperez

fperez Oct 17, 2011

Member

@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

fperez Oct 17, 2011

Member

@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!

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Oct 16, 2011

Member

@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.

Member

takluyver commented Oct 16, 2011

@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.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 16, 2011

Member

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.

Member

fperez commented Oct 16, 2011

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.

juliantaylor added some commits Sep 20, 2011

reenable multiline history for terminals
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
ensure history remains intact when recalling multiline entry
Remember previous history length to remove the correct amount
of single line entries
@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 18, 2011

Member

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!

Member

fperez commented Oct 18, 2011

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!

@fperez fperez merged commit 7d6edcb into ipython:master Oct 18, 2011

@juliantaylor

This comment has been minimized.

Show comment
Hide comment
@juliantaylor

juliantaylor Oct 18, 2011

Contributor

sorry, for not reacting for a while, I had some other priorities the last few days.
Thanks for merging.

Contributor

juliantaylor commented Oct 18, 2011

sorry, for not reacting for a while, I had some other priorities the last few days.
Thanks for merging.

minrk added a commit that referenced this pull request Oct 18, 2011

use unicode_to_str with readline.add_history
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.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

use unicode_to_str with readline.add_history
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment