Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Execution refactor #163

Merged
9 commits merged into from Oct 10, 2010

Conversation

Projects
None yet
3 participants
Owner

fperez commented Oct 8, 2010

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.

fperez added some commits Oct 3, 2010

@fperez fperez Fix bugs with multiline cells.
For cells whose last entry was itself multiline, we were executing the
raw input and not the ipython translated input, so multline cells with
ipython syntax were broken in this case.

Also fixed history management: we now correctly store raw/translated
information in the respective histories.
ce5f9e5
@fperez fperez Refactor multiline input and prompt management. 891019f
@fperez fperez Created HistoryManager to better organize history control.
This will put in a single location all history-related operations, and
allow us to simplify the main classes further.

Readline dependencies remain (for history saving/loading), this is
only a first refactoring step.
ba4b5e8
@fperez fperez Add support for accessing raw data to inputsplitter.
This is necessary to be able to use inputsplitter systematically
across all frontends, including plain terminal one.
dceccd9
@fperez fperez Continue refactoring input handling across clients.
Fixed a number of small but tricky bugs related to prompt numbering,
now in all cases I can see both terminal and gui client are fully
consistent regarding prompts.
048fef2
@fperez fperez Remove all direct shell access from logger.
History management was moved to the history manager.
2523ba0
@fperez fperez Finish removing spurious calls to logger and runlines.
Now logging is done only by the shell in run_cell, and most uses of
runlines have been replaced by the far cleaner run_cell.
0fc2838
Owner

ellisonbg commented Oct 9, 2010

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

Owner

fperez commented Oct 9, 2010

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

Owner

fperez commented Oct 9, 2010

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 fperez Document object interface to HistoryManger according to our conventions.
All our objects should have their interface documented at the class
level before the actual constructor.
f9355af
Owner

fperez commented Oct 9, 2010

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

@fperez fperez Update many names to pep-8: savehist -> save_hist.
The old names are still made available for backwards compatibility.
73cd5b2
Owner

fperez commented Oct 9, 2010

Done, nothing more from me for now!

Owner

minrk commented Oct 10, 2010

Reviewed, merged, committed, and pushed.

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

Owner

fperez commented Oct 10, 2010

Fantastic, many many thanks!

Owner

ellisonbg commented Oct 11, 2010

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!
Owner

fperez commented Oct 12, 2010

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

Owner

ellisonbg commented Oct 12, 2010

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?

Owner

fperez commented Oct 12, 2010

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.

Owner

fperez commented Oct 12, 2010

Done in 8b47b8a.

Owner

ellisonbg commented Oct 13, 2010

This looks great, thanks!

@jtriley jtriley pushed a commit to jtriley/ipython that referenced this pull request Jul 30, 2011

@fperez fperez Small fixes in response to code review for gh-163. 17444f9

@minrk minrk added a commit to minrk/ipython that referenced this pull request Jul 1, 2013

@minrk minrk Merge pull request #163 from Carreau/clean-exit
warn instead of crashing if wrong number of args
3481b8a

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

@fperez fperez Small fixes in response to code review for gh-163. ab1743a

This issue was closed.

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