Merge lp:~robert-kern/ipython/pretty into lp:ipython/0.11

Proposed by Robert Kern
Status: Merged
Merged at revision: not available
Proposed branch: lp:~robert-kern/ipython/pretty
Merge into: lp:ipython/0.11
To merge this branch: bzr merge lp:~robert-kern/ipython/pretty
Reviewer Review Type Date Requested Status
Fernando Perez Approve
Ville M. Vainio Approve
Review via email: mp+3780@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Robert Kern (robert-kern) wrote :

Check out the docstring of IPython/Extensions/ipy_pretty.py for instructions on how to use it. Give it a whirl, and let me know if anything goes wrong.

Revision history for this message
Ville M. Vainio (villemvainio) wrote :

Some quick thoughts:

- Use ip.options.pprint instead of rc
- Raise TryNext if pprint is not on (instead of doing repr on your own)
- Move pretty.py to IPython.external
- Call set_hook("result_display", func, priority=99) since this is a 'catch-all' hook that should have a low priority (a hook that was interested in specific types of objects can override it). 100 is the priority of default hook

I didn't try it yet, but this popped to my mind quickly.

review: Needs Fixing
lp:~robert-kern/ipython/pretty updated
1155. By Robert Kern

Merging trunk.

1156. By Robert Kern

Incorporate suggestions from Ville.

Revision history for this message
Robert Kern (robert-kern) wrote :

Done.

Revision history for this message
Ville M. Vainio (villemvainio) wrote :

Fine with me

review: Approve
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
lp:~robert-kern/ipython/pretty updated
1157. By Robert Kern <email address hidden>

BUG: Partial fix for the case of old-style extension types that do not descend from object, like the VTK types. Still need to work on getting a real MRO for them.

1158. By Robert Kern <email address hidden>

BUG: Refactor the extension a little bit in response to review. Added an activate() function. Exposed the registration functions. Exposed the example dtype_pprinter in code rather than docstring. Added some doctests (not running yet!).

Revision history for this message
Robert Kern (robert-kern) wrote :

On Sat, Mar 14, 2009 at 19:34, Fernando Perez <email address hidden> wrote:
> Review: Needs Fixing
> 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.

Done as activate(). We need a way to unset hooks in order to implement
deactivate().

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

Done.

> 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

I have not been able to get this to work. I get 0 tests with your
example. Anyways, the doctestable docstring is there.

--
Robert Kern

"I have come to believe that the whole world is an enigma, a harmless
enigma that is made terrible by our own mad attempt to interpret it as
though it had an underlying truth."
  -- Umberto Eco

Revision history for this message
Ville M. Vainio (villemvainio) wrote :

On Sun, Mar 15, 2009 at 2:34 AM, Fernando Perez <email address hidden> wrote:
> Review: Needs Fixing

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

See _ip.load??

--
Ville M. Vainio
http://tinyurl.com/vainio

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

On Sat, Mar 14, 2009 at 6:24 PM, Robert Kern
<email address hidden> wrote:
> On Sat, Mar 14, 2009 at 19:34, Fernando Perez <email address hidden> wrote:
>> Review: Needs Fixing
>> 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.
>
> Done as activate(). We need a way to unset hooks in order to implement
> deactivate().
>
>> 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.
>
> Done.

Thanks!

>> 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
>
> I have not been able to get this to work. I get 0 tests with your
> example. Anyways, the doctestable docstring is there.

Cool. You may need my trunk-dev for that to work though, there's a
lot of work on the testing machinery I've done there.

I have to run now, I'll return to this tomorrow...

In the meantime, if anyone has a chance to look at the other branches
pending review, that would be great :)

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

OK, go for it. Your doctests don't run yet even with my branch, but that's because I have a bug in handling multiline ipython inputs. Don't worry about that, let's merge this now, while I fix that issue in my branch. The tests will then get automatically picked up (I'll update with your fixes when you merge, and will make sure I pick them up correctly).

Thanks!

Robert: feel free to merge yourself if you want. I think only a *few* of us in the ~ip-dev team should be actually doing merges, to avoid possible problems with merge history. We should have a 'mergers' informally defined group and mentor people in for merge-into-trunk work gradually, but you have enough experience with all this game (more than me :), that I think it's fair to say that you're part of said informally defined group.

We should probably discuss on list this to have an explicit policy, on who handles the final merge steps, since it's the one place where there's a chance for conflicts or history problems to appear if many people try to do it simultaneously...

Anyway, that's a topic for the list. Merge away, making sure you have fully updated from trunk first.

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

"""
OK, go for it. Your doctests don't run yet even with my branch, but that's because I have a bug in handling multiline ipython inputs. Don't worry about that, let's merge this now, while I fix that issue in my branch.
"""

BTW, I just fixed that and added tests:

http://bazaar.launchpad.net/~fdo.perez/ipython/trunk-dev/revision/1177

So you could try to put your changes on top of my branch to run your tests.

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

Robert: I went ahead and merged it as-is, to reduce the risk of conflicts with my large one. Brian already reviewed that and it's almost ready to merge too.

Before 0.10, we can look into using _ip.load() as Ville suggested, to rationalize the extension loading API, but now at least the main code is in.

Thanks a lot for the work!

Subscribers

People subscribed via source and target branches