Code review comment for lp:~fdo.perez/ipython/trunk-dev

Here are my comments for the first 50 commits.

1223
====

application.py: Why are profile_name, ipython_dir set to None in class
definition?

1224
====

magic.py

(80,10): Really wierd problem with making Magic a new style class!

(1571,7): Why self.shell instead of self?

I think you move some of this to another file later on.

1225
====

Maybe this explains why profile_name and ipython_dir are set to None
in r1223. I think this makes sense.

1226, 1227, 1228
================

1229
====

ipapp.py

I like how the pylab enabling has been implemented in ipapp and iplib.

(540,12): The call to InteractiveTB.set_mode(mode=self.shell.xmode) should
be moved into a method of iplib.InteractiveShell that is called when
InteractiveShell.__init__ is called. There is probably already a method
that instantiates the InteractiveTB instance. The self.shell.xmode attribute
should already be set by the config system.

pylabtools.py:

This should probably be moved to IPython/lib like inputhook.py. The idea of
the lib subpackage is to help us keep core as small as possible. Eventually
core should not depend on the terminal or on anything outside of the stdlib.
The way I think of it is that maybe someday, core would make it into the std
lib itself. So lib is for non-optional things (optional things go into extensions)
that add additional functionality to IPython beyond the core.

1230
====

1231
====

completer.py

In protect_filename maybe a note on why it is needed to protect () on win32.

1232
====

Where did the decorators_trial.py go? I thought we were still using these
to give a single interface for skipping tests in nose/trial. Maybe a later
commit will explain this...

1233
====

Why is _paramtestpy2.py and _paramtestpy3.py named with a leading underscore?
I am guessing that they are imported in a way that is python version
dependent?

Include the standard headers (copyright, etc.) in these files.

1234
====

Yeh, 1, 2, 3, 4!

In test_decorators.py it looks like there is a reference to the twisted
testing decorators that vanished. How are we handling that now?

1235, 1236
==========

1237
====

In prefilter near line 158, why ip.shell._ofind, rather than ip._ofind?

1238, 1239
==========

1240
====

quitter.py, line 36, do we want to print "Bye" on exit. Seems a bit verbose.

1241
====

Great.

1242
====

I think the self.parent.parent can be replaced by self.shell.

1243
====

Can we return an empty string instead of 'Bye'?

1244, 1245
==========

1246
====

OK, this fixes issue with r1242.

1247
====

Great.

1248
====

Should we start using this on all attributes?

1249
====

Can we renamed user_config_ns to something that reflects how it is used by
%who? Like user_ns_exclude? Eventually, when magics are components, the who
magic itself should track this namespace and provide a simple API for adding
things to it.

1250
====

Is this the code that enables you to get rid of the default=NoConfigDefault
all over the place? If so that is great. It is important that most command
line options use NoConfigDefault as the default value.

1251
====

1252
====

Very nice!!!

1253
====

application.py:

Anytime things change in application.py, we need to make sure that all
apps (ipapp,ipcontrollerapp, ipengineapp, ipclusterapp) are updated and tested.

Do all of these apps now use the new intialize/_initialized stuff?

Do all of these apps use the new argv stuff?

The Application startup logic is already on the verge of being too complex.
The additional distinction between start/initialize makes it worse. I think
we can get by without initialize. See my comments below on ipdoctest.py.

ipapp.py:

I am a little uncomfortable with the shell_params argument. One of the big
questions I faced in designing the config system was when to pass things
to a class in the config object and when to pass them as a keyword argument.
My current thinking is the config object should probably be preferred over
passing arguments to __init__.

ipdoctest.py:

Do you really need all the logic provided by IPythonApp? I am wondering
if you can get by with just i) creating the InteractiveShell by hand or ii)
and custom Application that builds the InteractiveShell configured as you
need it.

The fact that you had to change the Application API (initialize) and pass
shell_params, makes me think that you are trying to fit a square peg (
the existing IPythonApp) into a round hole (the testing system).

In my mind, an Application should only be used to create a full blown
application that runs in a process on its own. I don't think you need all the
config file handling, command line parsing, etc that Application provides.
Also, if you use the full blown IPythonApp, you will end up reading all the
user's config files, etc. It seems like for testing purposes you want to have
a more controlled InteractiveShell creation process.

Couldn't you get by by doing:

* Creating a config object by hand that has the options you need.
* Directly instantiating InteractiveShell by hand passing it that config.

The other aspect of this, is that you had to rip make_user_namespaces
out of InteractiveShell. One of the things that made the previous version of
the code a mess was that so much logic was put outside of InteractiveShell.
Can you give more details about why you need to create the namespace
before instantiating InteractiveShell. I am wondering if we can put
make_user_namespaces back into InteractiveShell and add some additional logic to
InteractiveShell that would allow the user_ns and user_global_ns arguments
to __init__ to be more useful. It would be nice if you could simply
pass a raw dictionary (your ipnsdict() in ipdoctest.py) into these arguments
and have make_user_namespaces be able to work with that.

We may need to talk about these issues further.

1254, 1255, 1256, 1257, 1258, 1259, 1260, 1261
========================================

1262
====

I like the idea of passing "arguments" to the __init__ method of
ArgParseConfigLoader. The only thing that is awkward is how inheritance of
arguments works. In line 120 of application.py, you provide a message about
this, but it is still a little awkard. Could we figure out a hybrid approach
that allows us to still have proper argument inheritance?

application.py:

Overall, I like the idea of having the crashhandler at the application level.
But, the parallel computing applications (ipcontroller,ipengine,ipcluster)
all need to be updated to use this. These top level applications are not
tested, so it is likely that unless they have been tested by hand, that they
are broken.

These other applications have a much more subtle and complex startup because
of the interplay between the twisted logging and stdlib logging. In many
cases, I had to take great care to make sure the right logging system
printed error.

The other big difference is that the type of situations that "crashes" can
arise in these other apps is very different. For "ipython" a crash is very
(hopefully) rare, but for the parallel computing apps, these types of crashes
are quite common. They happen anytime a controller or engine fails to start
or stops. This I don't think that triggering the full crash handler will
always be appropriate. In most cases, I think basic logging is better. In
either case, we will have to decide how to integrate the app-level crash
handler with these other apps and fix these other apps.

All of this is further complicated by the fact that ipcluster calls ipengine/
ipcontroller. It would be quite awkward to have both trigger the crash handler.

But, I do think that the application level is the right place for the crash
handler.

iplib/prefilterfrontend.py

Yes! I am so glad we could get rid of this!

1263, 1264, 1265, 1266
======================

1267
====

Not pretty, but OK.

1268, 1269
==========

1270
====

test_magic.py:

I like to have the imports organized in this way, but the
stdlib/third-part/our own labels are a bit verbose.

iptest.py:

Lines 181, 183. Anytime you run a python program on Windows as an external
process, it is important to start python in unbuffered mode (-u). If you don't
you get super subtle bugs sometimes.

1271
====

pretty.py:

Line 133. An extension should *never* get IPython by calling get_ipython.
It MUST always be passed to load_ipython_extension by its caller. Otherwise
there is no guarantee it will get the right InteractiveShell instance.

Line 139: I see you added a return value to the extension loading API. Any
particular reason for this.

1272, 1273
==========