Execution refactor

[fperez]

OK, this big puppy is ready for review so we can merge it!

It's a ton of work in some of the ugliest parts of the code, but so far it looks good. I tested the merge into trunk and all tests pass, and in all usage I haven't found any regressions.

A few highlights:

• logger does no history management
• new history object
• much cleaner run_cell

much more...

I think it's good progress on one of our worst parts.

[ellisonbg]

I will get on this review ASAP. Thanks for doing this work!

[fperez]

For future reference, I'm pasting here comments made by Min when he reviewed it over email:

Min R-K's review:

I saw that you removed the coding marker at the top of a couple of files. Are we not using those anymore? You left them in some files that you edited, but removed from a couple. Should I just ignore that?

HistoryManager doesn't declare attributes above init.py per coding standards, are we soft about that as well?

interactiveshell.py:
should we add underscores in method names (savehist, reloadhist) per PEP8, or leave them as they are?
It's a bit funky that they look like this:
def savehist(self):
"""Save input history to a file (via readline library)."""
self.history_manager.save_hist()

I imagine that if we rename them, we would alias the oldnames, at least for a while.

otherwise it looks very good, great work! I have a couple little spacing/comment/docstring tweaks, but if you respond to those notes, I can do the merge.

-MinRK

[fperez]

My replies to Min's review:

• coding markers: sorry that it was a bit inconsistent, I wasn't paying too much attention. A while ago we added coding markers systematically because we had a few files with unicode characters explicitly in them. But we actually have very, very few of those: I've removed over time most instances of my last name being accented (Pérez). I guess I find the coding marks unnecessary noise most of the time, and without thinking much I nuked a few.

But I'm happy to revert that or agree on a different long-term policy. My preference would be to use the marks only when actually necessary (which would be pretty much only when doing testing that explicitly does unicode tests).

What do you guys think?

• History manager: yes, I totally dropped the ball here. The object should declare its interface before init, as we agreed. Sorry.
• +1 to changing savehist to save_hist (and other similar ones). We inherited a lot of those run-in names from old code, we should update now. Obviously we do need to keep the backwards compatible names around for a while.

Thanks for the review!!

[fperez]

OK, I just fixed the interface, I'll do the _ names now...

[fperez]

Done, nothing more from me for now!

[minrk]

Reviewed, merged, committed, and pushed.

There were a couple tiny tweaks, like a space after a comma, but that's it.

[fperez]

Fantastic, many many thanks!

[ellisonbg]

history.py

• Let's have ShadownHist take shell as the argument and get rid of its usage
  of ipapi.
• HistoryManager still depends on readline, can we eliminate this dependency
  if readline is not available?
• store_inputs makes me happy!

interactiveshell.py

• Let's make self.buffer and self.buffer_raw traitlets of type List, so they
  are type checked and have the defaults ([]) set by traitlets. This will
  also help us clear out init_instance_attrs.
• Same for self.execution_count.
• FIXME at the top of run_cell is outdated? Let's update the commits in this
  method, because it is pretty important and subtle.
• Should we remove runlines?

prefilter.py

• Yah, no more logging in there!

[fperez]

Thanks, Brian! Notes (I'm quoting by copy/paste from you):

history.py

* Let's have ShadownHist take shell as the argument and get rid of its usage of ipapi.

[done, good catch]

* HistoryManager still depends on readline, can we eliminate this dependency if readline is not available?

[I think we could, but it would take a fair bit of testing, and we'd have to manually implement ourselves those functions. Nothing hard, but it's not a small fix. For now we'll just keep on living with that, I'm afraid. There's only so much I can clean up one pass :) ]

* store_inputs makes me happy!

[ yes! I think overall we're in much better shape with this (even with the unicode problems, which I'll work on). The architecture is right.

interactiveshell.py

* Let's make self.buffer and self.buffer_raw traitlets of type List, so they are type checked and have the defaults ([]) set by traitlets. This will also help us clear out init_instance_attrs.

[ Actually those variables are slated for complete removal. Once we fully trust this system, we'll get rid of runlines, push_line and these buffers. I added a note, but won't bother changing the code since they'll just go away later.]

* Same for self.execution_count.

[ Done, good catch]
* FIXME at the top of run_cell is outdated? Let's update the commits in this method, because it is pretty important and subtle.
[ Yup. That fixme was pretty much the blueprint for what I did :) I had just forgotten to remove it]
* Should we remove runlines?
[ Eventually, most certainly. I think we're pretty close to being able to get rid of it, just not quite there yet. A bit more testing and shaking out of the new code, and we'll nuke it]

Thanks again! I just committed these small fixes in 17444f9

[ellisonbg]

Great, all of these things look fine. Thanks especially for the notes about buffer and buffer_raw and runlines. Could we add comments to the code about these things going away eventually?

[fperez]

Your wish is my command, says Fernando as he whips up his time machine to answer your request, last night :)

http://github.com/ipython/ipython/commit/17444f91d7191237e71714a12761c962fd9a6c1d#L1R375

But I'll add a 'pending removal' note to runlines and push_lines just now, to make sure all these entry points are clearly labeled as condemned buildings, pending demolition.

[fperez]

Done in 8b47b8a.

[ellisonbg]

This looks great, thanks!