Code review comment for lp:~robert-kern/ipython/pretty

Revision history for this message
Fernando Perez (fdo.perez) wrote :

A few comments. This is great, but our extensions setup right now is a mess, see my email on list. Why don't you modify it to wrap the activation hook into a

def load():
   ....

method for now, and we can change the API later in trunk if we come to a different naming convention.

I'd also suggest putting your numpy prettyprinter as real code, instead of in-docstring, so that users can just load it themselves from your extension.

Finally, can you provide some doctests? While currently the main iptest script skips extensions (too many of them break dangerously if loaded), they can be manually tested via

maqroll[scratch]> cat /home/fperez/ipython/ipython/IPython/Extensions/ipy_foo.pydef foo():
    """simple doctest

    In [1]: print 1+2
    3

    In [2]: 1+2
    Out[2]: 3
    """
    return 1

maqroll[scratch]> iptest IPython.Extensions.ipy_foo
----------------------------------------------------------------------
Ran 1 test in 0.002s

OK

###

So a simple docstring with some examples that do the most basic showing and testing would be great. I'm trying to be good at not putting *any* new code that's fully tested.

If you add some test code, I'll manually whitelist it in iptest for now, until we get to the point where we can stop blacklisting all extensions.

Otherwise, fine with me. Thanks!

review: Needs Fixing

« Back to merge proposal