[IPython-dev] Comments about IPython.frontend, getting ready for merge of ipython-sync-frontend

[Fernando Perez]

Howdy,

On Thu, Aug 7, 2008 at 12:18 PM, Gael Varoquaux
<gael.varoquaux at normalesup.org> wrote:
> Hi Brian,
>
> Thanks a lot for your comments, they are very valuable.
>
> I have to run to catch Stéfan at the airport, I just wanted to give an
> informal overview of the status of my work. I am currently battling with
> subprocess execution, including under windows. The hard stuff is the
> stdin capture, and it happens more often than I would like. Once I am
> done with that (hopefully today, though my afternoon will be busy working
> with Stéfan), I have to make sure that 'Ctrl-C' stops Python execution,
> which should be easy. After this I was planning to clean up the code, add
> comments and ask for merge.

I'm going to put in here my comments regarding the code in
kernel.core, and will send  a separate email regarding merge plans,
since Barry made some comments on that matter too.

Modulo any possible conflicts with Barry's changes that we might need
to sort out, I think we can also merge in what you've done, so we have
an integrated codebase to work off from at scipy.

My comments follow, some are generic about files that were in
kernel.core before your branch, and some are specific to your code.
Overall I don't see any problem though, so I think it's just a matter
of moving forward with minimal collisions.

====================================
 Comments about IPython.kernel.core
====================================

General
=======

We'll likely push this over as IPython.core, but I'd like to do this after we
get this release out.  It will be much easier to do this refactoring at SciPy,
where at the very least we'll have Brian, Gael, Stefan and myself present, plus
other possible volunteers.  For now it's OK to do the merge where the code is.

General naming comment: we have a ton of modules_with_underscores in their
names.  In trying to keep to pep-8, let's at least make an effort to not let
this grow unnecessarily (while keeping in mind pep-8's first advice, "A Foolish
Consistency is the Hobgoblin of Little Minds" :)  Note: I'm not being petty
here, I actually happen to like the package namespace to use that convention...

Some of my comments below relate to code in kernel.core in general, not
specifically to Gael's recent changes.


Testing
=======

Ultimately I'd like us to be able to run a substantial fraction of the test
suite against each frontend, to ensure that the basic session runs in each
correctly.  This won't probe the UI aspects, but it will ensure that all
frontends conform to the same uses of the core objects for namespace handling,
completion, history, etc.  If it's not possible right now to do this, it should
be a high priority to enable the current code to run the tests.  This might be
a job for scipy so we can do it together.

In the meantime, I concur with Barry's assessment of the need for more tests.
I expect we'll be refactoring like mad at Scipy, and doing so without tests is
like juggling dual-edge knifes with no handles.

Even if there's no time for detailed coverage, at least having a set of small,
near-trivial tests for each file that minimally call the various
functions/methods makes a big difference.  They may not cover every corner
case, but they ensure that key invariants are honored, and they remind you of
what API breakage you're likely to incur when moving stuff around.


display_formatter.py
====================

[ This file is in trunk, so the comments apply to all of us, not specifically
to Gael's changes ]

I'm assuming that the return value must always be a string (or None), right?
Since this is meant to declare the interface, it should be explicit about the
valid return types.


display_trap.py
===============

[ This file is in trunk, so the comments apply to all of us, not specifically
to Gael's changes ]

What exactly is the purpose of the callback list?  Since the callbacks don't
return anything, what exactly are they supposed to do?

The slight lack of tests/fuller docstrings already mentioned needs some work.
 On the topic of docstrings, let's keep to pep-8 convention: one-line summary,
blank, full text (using reST format).  There are GUI tools that use the
one-line summary, and I'd like the GUI ipython versions to provide nice api
summaries of objects.  This works better if you can assume that the first line
is readable by itself.  add_to_message doesn't conform to this, for
example. __init__ needs a docstring

In hook, shouldn't obj be stored first, in case an error is raised by a
callback?  And how are errors in callbacks handled?


fd_redirector
=============

I imagine this is the part you've been struggling with, from your comments.  We
certainly need this to gracefully handle i/o under GUIs.  Docstrings and tests
are critically needed though, and the docstrings should conform to the reST api
for sphinx.

All attributes should be declared in the constructor, even if they are set to
None with a comment of what they do and that their true value is given later
(such as in start() ).  The fact that Python allows us to create attributes at
any point doesn't mean we should do it in normal code, its a feature whose
valid use cases are in decorating/annotating external objects.  But classes we
write should declare their attribute API completely in the constructor, for
readability/comprehensibility reasons.

Is it necessary to call stop() in getvalue()?

file_like.py
============

- writelines should read simply "map(self.write,lines)".  Faster, cleaner,
shorter.

- why are the other methods implemented as 'pass'?  Wouldn't it be better not
  to have them?  Making them pass means they return None, which can lead to
  them being used by other code which will then strangely break when None is
  the wrong thing to get.


output_trap.py
==============

This one implements a smaller set of functionality than the original
OutputTrap.  Why?  Do we need to merge the two?  Right now we have in
core.shell uses of the old module and in core.interpreter, of the new, so
likely some sort of merge will be needed.


redirector_output_trap.py
=========================

No docstrings on constructor.


sync_traceback_trap.py
======================

No docstrings on constructor.   The top-level docstring could be a bit more
verbose.