Merge pull request #929 from juliantaylor/multiline-history

some readline multiline-history fixes and tests

* fixes crash when readline is absent (#911)
* do not save input from e.g. raw_input in history
* do not add empty or duplicate entries lines to the history.
* add tests for _replace_rlhist_multiline
* protect pyreadline from some incorrect assumptions of equivalence to regular readline

closes gh-911

IPython/frontend/terminal/tests/test_interactivshell.py

@@ -0,0 +1,161
+# -*- coding: utf-8 -*-
+"""Tests for the key interactiveshell module.
+
+Authors
+-------
+* Julian Taylor
+"""
+#-----------------------------------------------------------------------------
+#  Copyright (C) 2011  The IPython Development Team
+#
+#  Distributed under the terms of the BSD License.  The full license is in
+#  the file COPYING, distributed as part of this software.
+#-----------------------------------------------------------------------------
+
+#-----------------------------------------------------------------------------
+# Imports
+#-----------------------------------------------------------------------------
+# stdlib
+import unittest
+
+from IPython.testing.decorators import skipif
+
+class InteractiveShellTestCase(unittest.TestCase):
+    def rl_hist_entries(self, rl, n):
+        """Get last n readline history entries as a list"""
+        return [rl.get_history_item(rl.get_current_history_length() - x)
+                for x in range(n - 1, -1, -1)]
+
+    def test_runs_without_rl(self):
+        """Test that function does not throw without readline"""
+        ip = get_ipython()
+        ip.has_readline = False
+        ip.readline = None
+        ip._replace_rlhist_multiline(u'source', 0)
+
+    @skipif(not get_ipython().has_readline, 'no readline')
+    def test_runs_without_remove_history_item(self):
+        """Test that function does not throw on windows without
+           remove_history_item"""
+        ip = get_ipython()
+        if hasattr(ip.readline, 'remove_history_item'):
+            del ip.readline.remove_history_item
+        ip._replace_rlhist_multiline(u'source', 0)
+
+    @skipif(not get_ipython().has_readline, 'no readline')
+    @skipif(not hasattr(get_ipython().readline, 'remove_history_item'),
+            'no remove_history_item')
+    def test_replace_multiline_hist_disabled(self):
+        """Test that multiline replace does nothing if disabled"""
+        ip = get_ipython()
+        ip.multiline_history = False
+
+        ghist = [u'line1', u'line2']
+        for h in ghist:
+           ip.readline.add_history(h)
+        hlen_b4_cell = ip.readline.get_current_history_length()
+        hlen_b4_cell = ip._replace_rlhist_multiline(u'sourc€\nsource2',
+                                                    hlen_b4_cell)
+
+        self.assertEquals(ip.readline.get_current_history_length(),
+                          hlen_b4_cell)
+        hist = self.rl_hist_entries(ip.readline, 2)
+        self.assertEquals(hist, ghist)
+
+    @skipif(not get_ipython().has_readline, 'no readline')
+    @skipif(not hasattr(get_ipython().readline, 'remove_history_item'),
+            'no remove_history_item')
+    def test_replace_multiline_hist_adds(self):
+        """Test that multiline replace function adds history"""
+        ip = get_ipython()
+
+        hlen_b4_cell = ip.readline.get_current_history_length()
+        hlen_b4_cell = ip._replace_rlhist_multiline(u'sourc€', hlen_b4_cell)
+
+        self.assertEquals(hlen_b4_cell,
+                          ip.readline.get_current_history_length())
+
+    @skipif(not get_ipython().has_readline, 'no readline')
+    @skipif(not hasattr(get_ipython().readline, 'remove_history_item'),
+            'no remove_history_item')
+    def test_replace_multiline_hist_keeps_history(self):
+        """Test that multiline replace does not delete history"""
+        ip = get_ipython()
+        ip.multiline_history = True
+
+        ghist = [u'line1', u'line2']
+        for h in ghist:
+           ip.readline.add_history(h)
+
+        #start cell
+        hlen_b4_cell = ip.readline.get_current_history_length()
+		# nothing added to rl history, should do nothing
+        hlen_b4_cell = ip._replace_rlhist_multiline(u'sourc€\nsource2',
+                                                    hlen_b4_cell)
+
+        self.assertEquals(ip.readline.get_current_history_length(),
+                          hlen_b4_cell)
+        hist = self.rl_hist_entries(ip.readline, 2)
+        self.assertEquals(hist, ghist)
+
+
+    @skipif(not get_ipython().has_readline, 'no readline')
+    @skipif(not hasattr(get_ipython().readline, 'remove_history_item'),
+            'no remove_history_item')
+    def test_replace_multiline_hist_replaces_twice(self):
+        """Test that multiline entries are replaced twice"""
+        ip = get_ipython()
+        ip.multiline_history = True
+
+        ip.readline.add_history(u'line0')
+        #start cell
+        hlen_b4_cell = ip.readline.get_current_history_length()
+        ip.readline.add_history('l€ne1')
+        ip.readline.add_history('line2')
+        #replace cell with single line
+        hlen_b4_cell = ip._replace_rlhist_multiline(u'l€ne1\nline2',
+                                                    hlen_b4_cell)
+        ip.readline.add_history('l€ne3')
+        ip.readline.add_history('line4')
+        #replace cell with single line
+        hlen_b4_cell = ip._replace_rlhist_multiline(u'l€ne3\nline4',
+                                                    hlen_b4_cell)
+
+        self.assertEquals(ip.readline.get_current_history_length(),
+                          hlen_b4_cell)
+        hist = self.rl_hist_entries(ip.readline, 3)
+        self.assertEquals(hist, ['line0', 'l€ne1\nline2', 'l€ne3\nline4'])
+
+
+    @skipif(not get_ipython().has_readline, 'no readline')
+    @skipif(not hasattr(get_ipython().readline, 'remove_history_item'),
+            'no remove_history_item')
+    def test_replace_multiline_hist_replaces_empty_line(self):
+        """Test that multiline history skips empty line cells"""
+        ip = get_ipython()
+        ip.multiline_history = True
+
+        ip.readline.add_history(u'line0')
+        #start cell
+        hlen_b4_cell = ip.readline.get_current_history_length()
+        ip.readline.add_history('l€ne1')
+        ip.readline.add_history('line2')
+        hlen_b4_cell = ip._replace_rlhist_multiline(u'l€ne1\nline2',
+                                                    hlen_b4_cell)
+        ip.readline.add_history('')
+        hlen_b4_cell = ip._replace_rlhist_multiline(u'', hlen_b4_cell)
+        ip.readline.add_history('l€ne3')
+        hlen_b4_cell = ip._replace_rlhist_multiline(u'l€ne3', hlen_b4_cell)
+        ip.readline.add_history('  ')
+        hlen_b4_cell = ip._replace_rlhist_multiline('  ', hlen_b4_cell)
+        ip.readline.add_history('\t')
+        ip.readline.add_history('\t ')
+        hlen_b4_cell = ip._replace_rlhist_multiline('\t', hlen_b4_cell)
+        ip.readline.add_history('line4')
+        hlen_b4_cell = ip._replace_rlhist_multiline(u'line4', hlen_b4_cell)
+
+        self.assertEquals(ip.readline.get_current_history_length(),
+                          hlen_b4_cell)
+        hist = self.rl_hist_entries(ip.readline, 4)
+        # expect no empty cells in history
+        self.assertEquals(hist, ['line0', 'l€ne1\nline2', 'l€ne3', 'line4'])

IPython/core/interactiveshell.py

@@ -308,7 +308,7 class InteractiveShell(SingletonConfigurable, Magic):
         """
     )
     multiline_history = CBool(sys.platform != 'win32', config=True,
-        help="Store multiple line spanning cells as a single entry in history."
+        help="Save multi-line entries as one entry in readline history"
     )
 
     prompt_in1 = Unicode('In [\\#]: ', config=True)

IPython/frontend/terminal/interactiveshell.py

@@ -231,13 +231,30 class TerminalInteractiveShell(InteractiveShell):
 
     def _replace_rlhist_multiline(self, source_raw, hlen_before_cell):
         """Store multiple lines as a single entry in history"""
-        if self.multiline_history and self.has_readline:
-            hlen = self.readline.get_current_history_length()
-            for i in range(hlen - hlen_before_cell):
-                self.readline.remove_history_item(hlen - i - 1)
-            stdin_encoding = sys.stdin.encoding or "utf-8"
-            self.readline.add_history(py3compat.unicode_to_str(source_raw.rstrip(),
-                                        stdin_encoding))
+
+        # do nothing without readline or disabled multiline
+        if not self.has_readline or not self.multiline_history:
+            return hlen_before_cell
+
+        # windows rl has no remove_history_item
+        if not hasattr(self.readline, "remove_history_item"):
+            return hlen_before_cell
+
+        # skip empty cells
+        if not source_raw.rstrip():
+            return hlen_before_cell
+
+        # nothing changed do nothing, e.g. when rl removes consecutive dups
+        hlen = self.readline.get_current_history_length()
+        if hlen == hlen_before_cell:
+            return hlen_before_cell
+
+        for i in range(hlen - hlen_before_cell):
+            self.readline.remove_history_item(hlen - i - 1)
+        stdin_encoding = sys.stdin.encoding or "utf-8"
+        self.readline.add_history(py3compat.unicode_to_str(source_raw.rstrip(),
+                                    stdin_encoding))
+        return self.readline.get_current_history_length()
 
     def interact(self, display_banner=None):
         """Closely emulate the interactive Python console."""
@@ -255,13 +272,15 class TerminalInteractiveShell(InteractiveShell):
             self.show_banner()
 
         more = False
-        hlen_before_cell = self.readline.get_current_history_length()
 
         # Mark activity in the builtins
         __builtin__.__dict__['__IPYTHON__active'] += 1
 
         if self.has_readline:
             self.readline_startup_hook(self.pre_readline)
+            hlen_b4_cell = self.readline.get_current_history_length()
+        else:
+            hlen_b4_cell = 0
         # exit_now is set by a call to %Exit or %Quit, through the
         # ask_exit callback.
 
@@ -293,8 +312,8 class TerminalInteractiveShell(InteractiveShell):
                 try:
                     self.write('\nKeyboardInterrupt\n')
                     source_raw = self.input_splitter.source_raw_reset()[1]
-                    self._replace_rlhist_multiline(source_raw, hlen_before_cell)
-                    hlen_before_cell = self.readline.get_current_history_length()
+                    hlen_b4_cell = \
+                        self._replace_rlhist_multiline(source_raw, hlen_b4_cell)
                     more = False
                 except KeyboardInterrupt:
                     pass
@@ -322,9 +341,9 class TerminalInteractiveShell(InteractiveShell):
                     self.edit_syntax_error()
                 if not more:
                     source_raw = self.input_splitter.source_raw_reset()[1]
-                    self._replace_rlhist_multiline(source_raw, hlen_before_cell)
-                    hlen_before_cell = self.readline.get_current_history_length()
                     self.run_cell(source_raw, store_history=True)
+                    hlen_b4_cell = \
+                        self._replace_rlhist_multiline(source_raw, hlen_b4_cell)
 
         # We are off again...
         __builtin__.__dict__['__IPYTHON__active'] -= 1