Multiline history

[juliantaylor]

some readline multiline history fixes and tests

fixes crash when readline (gh-911)
do not save input from e.g. raw_input in history
do not add empty lines to the history.
add a testcase for _replace_rlhist_multiline
update the docstring for the configuration variable

[minrk]

Thanks! The new tests do pass on my Python 3.2.

[minrk]

Since the refs are still all in one call, this can still be a transient variable and arg to _replace_rlhist, rather than adding an attribute, right?

[juliantaylor]

yes, then it would have to return the history length which must be saved by the caller
I'm not sure which variant is better, both are prone to errors during refactoring/reuse.

[fperez]

I'd also prefer a variant with less global state added to the instance. Those interactiveshell objects are already huge, much too large to conveniently handle. We can't remove a lot of what they have for historical reasons, but we shouldn't add more to them.

When state is needed, our approach lately has been to refactor the functionality into sub-objects that at least hold all related state together, rather than putting more and more weight into the top-level objects.

If you don't want to embark onto such a refactoring with a new object to manage all readline-related state (what would actually be a worthwhile effort, but perhaps one to entertain post-0.12), I suggest keeping a more lightweight functional interface for now.

[minrk]

If we want this as-is, just with the hlen as a variable instead of an attribute, I can make that change in the merge. It's only a couple of lines.

[fperez]

That would be great, @minrk. Go for it. Thanks!

[takluyver]

We've also got a regression from 0.11 where consecutive duplicate entries are stored multiple times, so if you do:

In [1]: a = 1
In [2]: b = 2
In [3]: b = 2
In [4]:

Pressing up-arrow gets you b = 2 twice before you see a = 1. By default, readline seems to condense these consecutive duplicates. I think we need to check hlen > hlen_before_cell before we call add_history.

Thanks to @antoine-levitt for spotting this in our discussion on #943.

[juliantaylor]

interesting I was not aware readline did this by default. In bash you have to explicitly enable it.
checking the length will not work for multi line entries. One would have to loop over hlen-hlen_before and compare the entries.
Or is there a simpler way?

[takluyver]

I would have thought it would work for any cell - if readline detects a duplicate, I imagine it's simply not adding it to its history list, so hlen == hlen_before_cell. Although I guess it still falls over if the code in the cell has called raw_input(), because that would be added to readline history. I think the only reliable way to deal with that is to rejig the readline history before we run the cell.

I thought it had always ignored duplicates in bash for me, but maybe it's only since I tweaked my .inputrc earlier this year.

[juliantaylor]

two equal two-line entries are are ABAB pattern for readline, but it only consecutive entries (AAA...) are skipped by it.
One could also chose to ignore the issue for multiline entries to keep the code simple, these are generally less often entered multiple times so it won't matter much.

[takluyver]

Oh, yes, I see. Although the most plausible way to repeat a multiline cell exactly is to recall it from readline history via up-arrow, in which case it hopefully would be picked up as a duplicate (assuming it compares it with the same list we modify in _replace_rlhist_multiline). I think it's fine not to merge duplicate multiline cells if the user has actually typed them out again.

[fperez]

Yes, huge +1 to fixing the duplicate entries in readline history. I only noticed it recently, but it's super annoying. @takluyver, if it doesn't get fixed in this PR, could you open a separate issue for it then? I suspect we'll hear a lot of complains if we release 0.12 with that problem...

[takluyver]

I was just about to open an issue when I noticed this PR. I think it should be possible to resolve it here, but if not I'll remind everyone about it.

[juliantaylor]

for the single line entries it can be resolved here with an extra length check.
I did not change it to have no member state yet as at some point this must be refactored to allow third party applications to reuse it (e.g. accerciser, exaile and rabbitvcs are currently duplicating the interact code). removing the state would probably make the api more complicated. But I did not have time to look at this in detail.

[fperez]

@juliantaylor, it would be great if you can add the fix here, we can review the lot and merge this. Thanks!

[minrk]

Scope issue in the recent change:

When I try to run without readline (ipython --TerminalInteractiveShell.readline_use=False), I get:

/path/to/IPython/frontend/terminal/interactiveshell.pyc in interact(self=<IPython.frontend.terminal.interactiveshell.TerminalInteractiveShell object>, display_banner=False)
    325                      'IPython will resume normal operation.')
    326             except:
    327                 # exceptions here are VERY RARE, but they can be triggered
    328                 # asynchronously by signal handlers, for example.
    329                 self.showtraceback()
    330             else:
    331                 self.input_splitter.push(line)
    332                 more = self.input_splitter.push_accepts_more()
    333                 if (self.SyntaxTB.last_syntax_error and
    334                     self.autoedit_syntax):
    335                     self.edit_syntax_error()
    336                 if not more:
    337                     source_raw = self.input_splitter.source_raw_reset()[1]
    338                     self.run_cell(source_raw, store_history=True)
    339                     hlen_b4_cell = \
--> 340                         self._replace_rlhist_multiline(source_raw, hlen_b4_cell)
    341 
    342         # We are off again...
    343         __builtin__.__dict__['__IPYTHON__active'] -= 1
    344 
    345         # Turn off the exit flag, so the mainloop can be restarted if desired
    346         self.exit_now = False
    347 
    348     def raw_input(self, prompt=''):
    349         """Write a prompt and read a line.
    350 
    351         The returned line does not include the trailing newline.
    352         When the user enters the EOF key sequence, EOFError is raised.
    353 
    354         Optional inputs:
    355 

UnboundLocalError: local variable 'hlen_b4_cell' referenced before assignment

after my first execution.

[juliantaylor]

forgot to test without readline again ._.
fixed

[minrk]

Awesome, thanks. It's almost there. The tests that set multiline_history=True should be skipped on Windows, where multi-line history doesn't work with pyreadline, due to missing rl.remove_history_item().

In fact, we should probably even check for the existence of that method in _replace_rlhist_multiline(), to prevent users crashing IPython on Windows by setting multiline_history=True, where it won't work.

[fperez]

On Mon, Nov 7, 2011 at 11:12 AM, Min RK
reply@reply.github.com
wrote:

In fact, we should probably even check for the existence of that method in _replace_rlhist_multiline(), to prevent users crashing IPython on Windows by setting multiline_history=True, where it won't work.

Good idea: someone may actually share their ipython config between
windows and linux if they have it on a shared filesystem. I do that
when I run in windows VMs that see my linux $HOME, so this is not just
hypothetical. It would be super annoying to have to change every
time.

Cheers,

f

[minrk]

This line will fail on Windows, where the function already isn't defined.

[minrk]

tested, and apparently works on my Windows VM, and on OSX with/without readline. merging.

[antoine-levitt]

I'm still seeing duplicate lines in ipython's own history (the readline history in one session is fine, but upon restarting ipython, duplicates are present)

[takluyver]

@antione-levitt: I think that's a separate issue (and I can replicate it on 0.11, before any of this). I've made issue #1000 so we don't forget it.

As an aside: that makes 1000 issues in a bit over a year, minus however many were transferred over from Launchpad.

[fperez]

On Mon, Nov 14, 2011 at 7:02 AM, Thomas
reply@reply.github.com
wrote:

As an aside: that makes 1000 issues in a bit over a year, minus however many were transferred over from Launchpad.

And it makes you the proud winner of a Scipy 2009 T-Shirt and a Scipy
India 2010 one! I have a few T-shirts on my desk from old
conferences and I'd decided to give two to the submitter of #1000 :)
Glad it's you, since you've done more than your fair share to earn
them!

Email me your home address at my Berkeley email and I'll post them :)

Cheers,

f