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

Barry Wark barrywark@gmail....
Fri Aug 8 09:24:16 CDT 2008


On Fri, Aug 8, 2008 at 5:05 AM, Fernando Perez <fperez.net@gmail.com> wrote:
> Howdy,
>
> On Thu, Aug 7, 2008 at 12:18 PM, Gael Varoquaux
> <gael.varoquaux@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.

I'd like to play devil's advocate for a minute here: I think display
formatting should be done exclusively in the frontend. If not,
frontends have no choice what to do with, e.g. matplotlib figures.
Putting it in core smells of the poor separation of concerns we have
in the ipython0 shell.

>
>
> 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.

Again, playing Devi's advocate: Much of what appears to be the
function of output_trap, sync_traceback_trap, and display_trap could
be handled in frontend by posting notifications from the Interpreter.
Doing so would separate these UI concerns (i.e. what to do with the
Interpreter's output) from the core function (producing that output).

> _______________________________________________
> IPython-dev mailing list
> IPython-dev@scipy.org
> http://lists.ipython.scipy.org/mailman/listinfo/ipython-dev
>


More information about the IPython-dev mailing list