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