IPython

Merge lp://qastaging/~doerwalter/ipython/ipipe-fix into lp://qastaging/ipython/0.11

  1. ipipe-fix
  2. Merge into trunk
Proposed by Walter Dörwald
Status: Merged
Merged at revision: not available
Proposed branch: lp://qastaging/~doerwalter/ipython/ipipe-fix
Merge into: lp://qastaging/ipython/0.11
Diff against target: None lines
To merge this branch: bzr merge lp://qastaging/~doerwalter/ipython/ipipe-fix
Reviewer Review Type Date Requested Status
Fernando Perez Approve
Brian Granger Approve
Review via email: mp+4945@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Jörgen Stenarson (jorgen-stenarson) wrote :

The fix looks ok to me. However there does not seem to be any tests for any of the extensions.

/Jörgen

Revision history for this message
Walter Dörwald (doerwalter) wrote :

Jörgen Stenarson wrote:

> The fix looks ok to me. However there does not seem to be any tests for any of the extensions.

True, there are no tests. Writing tests for the interactive parts of
ipipe would be difficult (how does one test a curses application?).
However there could be tests for the non-interactive parts.

Do we need tests for ipipe before the 0.10 release?

Servus,
   Walter

Revision history for this message
Brian Granger (ellisonbg) wrote :

Yes, testing a curses app is very non-trivial. If tests for the non-interactive parts are easy to add, then it would be nice. But adding tests to existing things like this can be a medium term project. But, a good guideline is that when we touch old code for any purpose, we add tests.

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

Walter, I agree with Brian in approving this despite the lack of tests for the curses part.

But precisely because testing the UI parts is in general so hard (equally problematic with GUI toolkits), the best approach is to really try and put as little logic as absolutely possible in the UI-dependent code, so that *all the rest* can be tested as a library.

So for future reference, let's try to get into the habit of really testing everything we possibly can.

I actually went ahead and merged your branch then. It would be great if you could refactor the ipipe machinery so most of it was testable standalone.

Thanks for the code!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'IPython/Extensions/ipipe.py'
2--- IPython/Extensions/ipipe.py 2008-08-02 09:04:09 +0000
3+++ IPython/Extensions/ipipe.py 2009-03-26 18:51:37 +0000
4@@ -114,6 +114,11 @@
5 items.reverse()
6 return items
7
8+try: # Python 2.4 compatibility
9+ GeneratorExit
10+except NameError:
11+ GeneratorExit = SystemExit
12+
13 try:
14 import pwd
15 except ImportError:
16@@ -666,7 +671,7 @@
17 try:
18 for x in func(mode):
19 yield x
20- except (KeyboardInterrupt, SystemExit):
21+ except (KeyboardInterrupt, SystemExit, GeneratorExit):
22 raise
23 except Exception:
24 yield (astyle.style_default, repr(item))
25@@ -840,20 +845,20 @@
26 """
27 Convert an attribute descriptor string to a real descriptor object.
28
29- If attr already is a descriptor object return if unmodified. A
30+ If attr already is a descriptor object return it unmodified. A
31 ``SelfDescriptor`` will be returned if ``attr`` is ``None``. ``"foo"``
32 returns an ``AttributeDescriptor`` for the attribute named ``"foo"``.
33 ``"foo()"`` returns a ``MethodDescriptor`` for the method named ``"foo"``.
34 ``"-foo"`` will return an ``IterAttributeDescriptor`` for the attribute
35 named ``"foo"`` and ``"-foo()"`` will return an ``IterMethodDescriptor``
36- for the method named ``"foo"``. Furthermore integer will return the appropriate
37+ for the method named ``"foo"``. Furthermore integers will return the appropriate
38 ``IndexDescriptor`` and callables will return a ``FunctionDescriptor``.
39 """
40 if attr is None:
41 return selfdescriptor
42 elif isinstance(attr, Descriptor):
43 return attr
44- elif isinstance(attr, str):
45+ elif isinstance(attr, basestring):
46 if attr.endswith("()"):
47 if attr.startswith("-"):
48 return IterMethodDescriptor(attr[1:-2])

Subscribers

People subscribed via source and target branches