Adapt magic commands to new history system. #261

Closed
wants to merge 9 commits into
from

Projects

None yet

3 participants

@takluyver
IPython member

This grew from issue ipython/ipython#245. Various magic commands weren't working properly with the new history system: %edit, %macro, and %hist.

Among various minor troubles, selecting a range of lines (%macro test 2-5) numbered from the beginning of the history, so didn't match up with the current line numbers. I've approached this by adding a session_offset attribute to the history manager. This has the added benefit that we no longer need to store a blank history entry so we can count lines from 1.

Along the way, I simplified and modernised parts of the code, including using basestring over StringTypes and .isdigit() over an equivalent regex.

@fperez fperez commented on the diff Feb 9, 2011
IPython/core/history.py
"""Create a new history manager associated with a shell instance.
+
@fperez
fperez Feb 9, 2011

Docstring should be written with

Parameters
---------------
load_history : bool
 ....

as per our doc guidelines.

@fperez fperez commented on an outdated diff Feb 9, 2011
IPython/core/history.py
@@ -77,6 +80,9 @@ class HistoryManager(object):
# pre-processing. This will allow users to retrieve the input just as
# it was exactly typed in by the user, with %hist -r.
self.input_hist_raw = []
+
+ # Offset so the first line of the current session is #1
+ self.session_offset = -1
@fperez
fperez Feb 9, 2011

Since this is a new attribute, it should be listed at the class level, for details see: http://ipython.scipy.org/doc/nightly/html/development/coding_guide.html#attribute-declarations-for-objects

@fperez fperez commented on the diff Feb 9, 2011
IPython/core/history.py
else:
- hist[i] = input_hist[i]
- if not hist:
@fperez
fperez Feb 9, 2011

Why was this if statement removed? Can the condition not happen anymore?

@takluyver
takluyver Feb 9, 2011

It couldn't happen before, because of the blank first entry in the hist lists (they started as [''], which evaluates as True). With these changes, it can happen if you request the entire history (using index=None) when it's empty. The Qt console requests the entire history when it starts up. To my mind, it makes more sense to return the empty list in that case than to raise an IndexError. The IndexError isn't currently caught anywhere, so it crashes the kernel.

@fperez
IPython member

This looks great, many thanks! The fixes and cleanups/modernizations are excellent.

I have a few minor notes that I made inline, once those addressed the only other overall concern that I have is whether you could add a couple of small tests. We're trying to be good about adding new (even fairly trivial ones are better than nothing) tests as we retrofit/fix old code. Though I admit that writing a good test for something that depends so much on actual line numbers (like calls to %macro) is tricky... But if you can come up with one, that would be great. If not, no big deal, the code is already a major improvement.

@takluyver
IPython member

Alright, I've made the first two changes you suggested, and tried to explain the removal of the if statement. I'll have a look at adding some tests this evening. Thanks for looking over this.

@takluyver
IPython member

And there are some tests. I've not attempted to test every bit: %edit, in particular, hasn't got any coverage. But this verifies that the session offset works, and that the %save command picks up on that. There's also a basic test of %macro.

@rkern

Do we have an idea of what we want to do with the In variable? Currently, it just exposes the input_hist_parsed attribute (a plain list) on the history manager without regard to the session offset. We used to have a fancy InputList that would implement slicing by joining the sliced lines together. This was nice for saving out lines to a file.

@fperez
IPython member

No, I have to admit I haven't thought this one through very well yet. I suggested to Satra in India we simplify things out by getting rid of the fancy InputList class, looking for a codebase with less custom objects everywhere. But perhaps that was a little bit premature?

Should we revive the InputList object and adjust the code to use it instead? It was really just a simple list subclass with the string joining semantics upon slicing, so bringing it back would be very easy...

I'm totally open to ideas on this front, and sorry if some of the early changes had unintended consequences.

@rkern

Well, the slicing bit is optional (but nice!). The important thing is that In[i] should work for the current session's numbering, however that gets arranged. A custom object is the simplest way to achieve that. It probably should be a non-list object wrapped around input_hist_parsed rather than a list subclass used as input_hist_parsed.

@rkern

That said, the In issue can be dealt with later. This fix should be merged ASAP. It's really annoying!

@fperez
IPython member

OK, the changes here look good, but I've just noticed that the actual reloading of previous history sessions gets broken by this branch, so something is amiss. Try opening the Qt console and issuing a few commands, then close it and open it again. The history should reload, and in master it works. But not in this branch...

Let's not merge this until we understand why, and ideally, we add a test that would catch this automatically in the first place...

@takluyver
IPython member

OK, I know what it is. The Qt console requests the history from the kernel. With these modifications, requesting the history without specifying a range gets you just this session's history. I'll try to see what the best way to handle that is.

@takluyver takluyver Add option to get_history to determine whether to retrieve the curren…
…t session or the entire history. Qt console on startup requests entire history.
ead117c
@takluyver
IPython member

I've added another keyword parameter to get history, this_session=True. The Qt console makes its initial history request with the parameter set to False, and in combination with index=None, this retrieves the entire history, including previous sessions.

While this works, it's beginning to feel a bit inelegant. Should I look at rethinking how history is stored and retrieved? I can do that either in this pull request, or as a separate one. Does anyone have any ideas of how it might best be organised?

@fperez
IPython member

Question: what's the status of this pull request now that the sqlite history work is on its way? That's mostly backend work, but I can imagine you may have also made front-side changes in that branch that may override this branch...

@takluyver
IPython member

This is superseded by sqlite history, so if we're definitely going down that route, this can be closed, along with #235 and #236. I'd left them open while I was still working to convince people that sqlite history was the way forward.

@takluyver
IPython member

This is superseded by SQLite history, which I've just merged.

@takluyver takluyver closed this Mar 24, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment