Rewrite code to restore readline history after an action #319

Merged
merged 4 commits into from Apr 3, 2011

Conversation

Projects
None yet
3 participants
Owner

takluyver commented Mar 27, 2011

The code to restore readline history (used for subprograms including %debug) called the save_history and reload_history methods, which were removed with my history refactoring. This replaces that code with a context manager, ip.readline_no_history, to do the same thing.

Closes gh-318

Contributor

efiring commented Mar 27, 2011

Works for me, thanks.

Owner

fperez commented Mar 27, 2011

Hey Thomas, this works in the sense of clearing the bug, but it doesn't restore the intended behavior.

What we need to do is to repopulate the readline history upon re-entry to ipython. So the history_saving_wrapper should still be there, it just needs to repopulate the readline history from the sqlite database when exiting the trapped sub-program.

Does this make sense?

The idea is that if you enter pdb:

In [2]: debug
> /home/fperez/scratch/err.py(9)()
      7 f"""
      8 
----> 9 foo33

ipdb> l
      4 d
      5 sf
      6 asd
      7 f"""
      8 
----> 9 foo33

ipdb> q

then at the next ipython prompt, up-arrow shouldn't show 'q', it should show 'debug'. Otherwise, if you had a long pdb session, now you'll have to slog through all the pdb commands (which don't make sense in ipython) to get to your previous meaningful ipython command.

Owner

takluyver commented Mar 27, 2011

OK, this context manager seems to work. I'd like to refine it a bit, though - at present, it hits the database no matter what. If when we enter the block, we fingerprint the readline history in some way (e.g. can we get the last 10 items?), then we can avoid reloading the history in the cases where the block needed no user input.

Owner

takluyver commented Mar 28, 2011

This should improve performance a bit: if the last 10 entries in readline are the same as when we entered the block, it doesn't bother clearing and refilling readline.

Owner

fperez commented Apr 3, 2011

This is now great, many thanks! Since I have it ready here, I'll just merge it now and it will get closed.

@efiring, thanks a lot for the bug report!

@fperez fperez merged commit 4851de4 into ipython:master Apr 3, 2011

Owner

takluyver commented Apr 3, 2011

The only thing I should mention is something I saw on Stackoverflow - do we want to offer the user a version of raw_input wrapped with this, so that their input doesn't get added to readline history?

Owner

fperez commented Apr 3, 2011

Sorry, not sure what you mean... Do you have a link to the SO discussion so I can understand better?

Owner

takluyver commented Apr 3, 2011

Here:
http://stackoverflow.com/questions/5506728/remove-items-from-ipythons-history

In short: what we do for subprograms and %debug, we could also do for calls
to raw_input, or even for all user code. That way, when the user code asks
for input, what they type doesn't go into readline history.

Owner

fperez commented Apr 3, 2011

Ok, this sounds fine, but it should be exposed as a separate method.
We shouldn't do this trapping by monkeypatching the system's
raw_input. Exposing a utility function that wraps this functionality
for people to call is OK, though...

Owner

takluyver commented Apr 3, 2011

OK. Is the best place to expose it as a method on the InteractiveShell? Or on the TerminalInteractiveShell, since we're not using readline elsewhere? TIS.user_raw_input() ? Or we could just execute user code inside the context manager, so that anything they type before our next prompt is dropped from readline.

Owner

fperez commented Apr 3, 2011

On Sun, Apr 3, 2011 at 10:37 AM, takluyver
reply@reply.github.com
wrote:

OK. Is the best place to expose it as a method on the InteractiveShell? Or on the TerminalInteractiveShell, since we're not using readline elsewhere? TIS.user_raw_input() ? Or we could just execute user code inside the context manager, so that anything they type before our next prompt is dropped from readline.

I think a TIS method is the right place. I don't want to do it by
default, because concievably someone could want to have their
application actually fill readline history. And in that case, if our
context manager is hardwired it becomes very difficult for them to
change that, short of reimplementing core ipython methods.

So I think an easily usable public api for this is the right approach,
and people who care enough about this can call that instead of plain
raw_input.

Question, is it possible to make this a standalone function instead of
a method of TIS? Or does it really need to access the shell object?
If it can be made a standalone function (without playing nasty tricks
with globals) that would be best, otherwise a TIS method seems like
the best alternative.

Owner

takluyver commented Apr 3, 2011

I think the crucial link to the shell is the readline_no_record decorator.That could be made into a global variable, although I know some people frown on that practice. Other than that, I believe the shell's readline attribute is just a reference to the imported readline module.

Owner

fperez commented Apr 3, 2011

Well, what's important is not to expose as a global module-level
variable anything that has references to a running ipython shell
object. In the past we've had (again, the usual historical mess) a
lot of that, and it causes many problems. Assuming there's a single
'master' ipython object makes test isolation hard and introduces nasty
hidden effects when embedding.

Now, if the only global is the self.readline thing that points to a
module, that probably can just be done away with. In fact, I think we
have an open ticket somewhere about that precisely, there's really no
good architectural reason for that to be there.

In summary, if this can be done as a standalone function that doesn't
really depend on the existance of any running ipython kernel, that
would be the ideal solution.

Owner

takluyver commented Apr 3, 2011

OK, I'll look into it, and make a new pull request.

Owner

fperez commented Apr 3, 2011

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment