IPython

Merge lp://qastaging/~fdo.perez/ipython/trunk-dev into lp://qastaging/ipython/0.11

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

Small fix for https://bugs.launchpad.net/ipython/+bug/353003

Revision history for this message
Jörgen Stenarson (jorgen-stenarson) wrote :

in test_iplib.py you have a new function test_user_setup() where the comment says: "This should basically run without errors or output".

But there are no checks for there to be no output as far as I can see.

Also I find it a bit distasteful to have two variables that only differ in case (ip and IP) it is not clear what they contain unless you look them up.

/Jörgen

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

Because test_user_setup() will be called by various parts of IPython (outside of the core even), do you think that test_user_setup() should be moved outside of iplib.py? Maybe this can wait for the upcoming re-organization.

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

Because test_user_setup() will be called by various parts of IPython (outside of the core even), do you think that test_user_setup() should be moved outside of iplib.py? Maybe this can wait for the upcoming re-organization.

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

I guess you should look at our comments before this gets merged. We should save "Approved" for things that are ready to be merged as is.

review: Needs Fixing
lp://qastaging/~fdo.perez/ipython/trunk-dev updated
1185. By Fernando Perez

Make user_setup a top-level function in iplib and add better tests.

This function has grown way too big and it needs a refactoring, but at least
now it's a standalone thing (it didn't need to be a method) and can be
called in a non-interactive fashion.

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

> in test_iplib.py you have a new function test_user_setup() where the comment
> says: "This should basically run without errors or output".
>
> But there are no checks for there to be no output as far as I can see.

I added a non-interactive mode that ensures no output to the screen (though nose caught that anyway) and also a few more explicit sanity checks. Though the main point initially was at least to verify that it ran without error, now there's a little more robust checking.

>
> Also I find it a bit distasteful to have two variables that only differ in
> case (ip and IP) it is not clear what they contain unless you look them up.

Yes, that's ugly but I kept that convention for historical reasons. It's better explained in the comments.

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

> I guess you should look at our comments before this gets merged. We should
> save "Approved" for things that are ready to be merged as is.

Yup. I moved it as a top-level function in iplib so we can call it from anywhere. It also has a new 'interactive' keyword so it can be safely used without expecting user interaction in tests.

lp://qastaging/~fdo.perez/ipython/trunk-dev updated
1186. By Fernando Perez

Fix https://bugs.launchpad.net/ipython/+bug/348466

The fix is not ideal, but at least it makes %timeit work. I'd like to have
a better solution though in the long term.

1187. By Fernando Perez

- Make ipdoctest a little cleaner by giving it separate option names.

This is in an attempt at better isolating it from the rest of nose, because
we are seeing intermittent Twisted errors when it is enabled. However, I
still see them sometimes.

- Also, make the reference counting tests a little less verbose.

1188. By Fernando Perez

Add note about Twisted bug to the docs.

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

Are all of these things ready to review? Assuming they are...

r1185:

Looks great. Eventually we can move user_setup to the config package.

r1186:

Bummer this doesn't always work, but the fix is fine.

r1187:

ipdoctestExtension -> ipdoctest_extension

r1188:

Good idea to add this info to the docs.

Why don't you go ahead and do the merge. Also, have you reverted the ref. bug that jdh found?

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

Once this is merged, I will make ipcluster/ipengine/ipcontroller use the new user_setup to fully resolve the original bug.

lp://qastaging/~fdo.perez/ipython/trunk-dev updated
1189. By Fernando Perez

Fix naming convention to PEP 8.

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

> ipdoctestExtension -> ipdoctest_extension

Thanks for catching that, I inherited it from the original nose conventions.

> Why don't you go ahead and do the merge. Also, have you reverted the ref. bug
> that jdh found?

Ouch. Kind of, I think, but it's VERY nasty, and my fix will just be a workaround.

I'm writing separately to the list about this because it's a looong story (I've worked on it for about 12 hours since yesterday, with a lot of help from Matthew Brett, and only now do we have somewhat of an understanding of what to do).

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

> Ouch.  Kind of, I think, but it's VERY nasty, and my fix will just be a workaround.
>
> I'm writing separately to the list about this because it's a looong story (I've worked on it for about 12 hours since yesterday, with a lot of help from Matthew Brett, and only now do we have somewhat of an understanding of what to do).

Sounds crazy and thanks again. I will look for the email you send to
the list. I am always amazed at how tough some of the bugs we find
are.

Cheers,

Brian

--
Brian E. Granger, Ph.D.
Assistant Professor of Physics
Cal Poly State University, San Luis Obispo
<email address hidden>
<email address hidden>

lp://qastaging/~fdo.perez/ipython/trunk-dev updated
1190. By Fernando Perez

Add test that fails for reference counting problem

1191. By Fernando Perez

Work again on bug 269966.

I think this time we do have a robust fix. I added tests that do fail after
John Hunter's example, so if this one ever tries to return, we should catch
it right away.

Very, very nasty bug...

Revision history for this message
Jörgen Stenarson (jorgen-stenarson) wrote :

Ran testsuite successfully on windows on python 2.5 from r1191.

There are many places where there is no space after a comma in tuples and function calls.
I've found it in the following revs: 1191, 1190, 1187, 1186, 1185, 1184, 1183

r1191 looks good overall.

 - line 1588 in magic.py, is it intentional to just comment out this line? The same kind of change further down you removed the line completely.

 - Is the refbug.py script in the tests directory intended to be a test or something else? Looks like it's used from magic_test.py. Perhaps this could be reflected in the docstring of refbug.py.

r1189 looks good

r1187 Some indentations obj_del.py are not a multiple of 4 spaces.

r1186 looks good

/Jörgen

review: Approve
lp://qastaging/~fdo.perez/ipython/trunk-dev updated
1192. By Fernando Perez

Use len and str from builtins in computing dynamic prompts.

Closes: https://bugs.launchpad.net/bugs/355837

1193. By Fernando Perez

Merging from upstream

1194. By Fernando Perez

more details (comments) on unicode bug

1195. By Prabhu Ramachandran

Fix %timeit for slow functions, by Prabhu Ramachandran.

From his submission on the list:

Lets say you have a function that takes 2 seconds to evaluate. With the
original code you are going to run that function 10 times regardless of the
fact that it already takes 2 seconds and with the original code, timing this
will take at least 60 seconds (with repeat=3) which is way too much.

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

Howdy,

> Ran testsuite successfully on windows on python 2.5 from r1191.

Great, many thanks!

> There are many places where there is no space after a comma in tuples and
> function calls.
> I've found it in the following revs: 1191, 1190, 1187, 1186, 1185, 1184, 1183

OK, I've fixed a few of them, though I didn't go crazy (a lot of them were from moving old code around). But I'll keep it in mind for the future (esp. since I tend to be the one beating people upside the head with pep8 :)

I have to say though, that for certain cases, I think that *not* using whitespace after commas can actually make things more readable. For example, I honestly find

a, (1,2,3), b

*much* nicer to read than

a, (1, 2, 3), b

because I think it makes the grouping clearer. So I think that PEP-8, with a small dose of common sense, should keep us pretty happy...

> r1191 looks good overall.
>
> - line 1588 in magic.py, is it intentional to just comment out this line? The
> same kind of change further down you removed the line completely.

Leftover junk, thanks.

> - Is the refbug.py script in the tests directory intended to be a test or
> something else? Looks like it's used from magic_test.py. Perhaps this could be
> reflected in the docstring of refbug.py.

Added notes.

> r1189 looks good
>
> r1187 Some indentations obj_del.py are not a multiple of 4 spaces.

Oops, thanks for catching that. Fixed.

>
> r1186 looks good

OK, I'm going to go ahead and merge. This has been reviewed pretty extensively, and the only other thing I added was Prabhu's little one-line fix to %timeit, which doesn't worry me at all.

The fixes in my branch have been holding back other stuff, so I'll just put it in.

Many, many thanks to you guys for the careful reviews! I'll do my part now reviewing others...

lp://qastaging/~fdo.perez/ipython/trunk-dev updated
1196. By Fernando Perez

Update version numbers for 0.10 dev

1197. By Fernando Perez

Small formatting fixes to address Jorgen's last code review.

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

Great! Thanks everyone for helping review.

Cheers,

Brian

On Mon, Apr 13, 2009 at 11:41 PM, Fernando Perez <email address hidden> wrote:
> Howdy,
>
>> Ran testsuite successfully on windows on python 2.5 from r1191.
>
> Great, many thanks!
>
>> There are many places where there is no space after a comma in tuples and
>> function calls.
>> I've found it in the following revs: 1191, 1190, 1187, 1186, 1185, 1184, 1183
>
> OK, I've fixed a few of them, though I didn't go crazy (a lot of them were from moving old code around).  But I'll keep it in mind for the future (esp. since I tend to be the one beating people  upside the head with pep8 :)
>
> I have to say though, that for certain cases, I think that *not* using whitespace after commas can actually make things more readable.  For example, I honestly find
>
> a, (1,2,3), b
>
> *much* nicer to read than
>
> a, (1, 2, 3), b
>
> because I think it makes the grouping clearer.  So I think that PEP-8, with a small dose of common sense, should keep us pretty happy...
>
>
>> r1191 looks good overall.
>>
>>  - line 1588 in magic.py, is it intentional to just comment out this line? The
>> same kind of change further down you removed the line completely.
>
> Leftover junk, thanks.
>
>>  - Is the refbug.py script in the tests directory intended to be a test or
>> something else? Looks like it's used from magic_test.py. Perhaps this could be
>> reflected in the docstring of refbug.py.
>
> Added notes.
>
>> r1189 looks good
>>
>> r1187 Some indentations obj_del.py are not a multiple of 4 spaces.
>
> Oops, thanks for catching that.  Fixed.
>
>>
>> r1186 looks good
>
>
> OK, I'm going to go ahead and merge.   This has been reviewed pretty extensively, and the only other thing I added was Prabhu's little one-line fix to %timeit, which doesn't worry me at all.
>
> The fixes in my branch have been holding  back other  stuff, so  I'll just put it in.
>
> Many, many thanks to you guys for the careful reviews!  I'll do my part  now reviewing others...
> --
> https://code.launchpad.net/~fdo.perez/ipython/trunk-dev/+merge/5098
> You are reviewing the proposed merge of lp:~fdo.perez/ipython/trunk-dev into lp:ipython.
>

--
Brian E. Granger, Ph.D.
Assistant Professor of Physics
Cal Poly State University, San Luis Obispo
<email address hidden>
<email address hidden>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'IPython/iplib.py'
2--- IPython/iplib.py 2009-03-16 23:58:42 +0000
3+++ IPython/iplib.py 2009-04-01 09:55:09 +0000
4@@ -1125,6 +1125,11 @@
5 print >> Term.cout
6 print '*'*70
7
8+ # Install mode should be re-entrant: if the install dir already exists,
9+ # bail out cleanly
10+ if mode == 'install' and os.path.isdir(ipythondir):
11+ return
12+
13 cwd = os.getcwd() # remember where we started
14 glb = glob.glob
15 print '*'*70
16
17=== modified file 'IPython/tests/test_iplib.py'
18--- IPython/tests/test_iplib.py 2009-03-16 01:38:04 +0000
19+++ IPython/tests/test_iplib.py 2009-04-01 09:55:09 +0000
20@@ -3,15 +3,25 @@
21
22 import nose.tools as nt
23
24+# Useful global ipapi object and main IPython one
25+ip = _ip
26+IP = ip.IP
27+
28
29 def test_reset():
30 """reset must clear most namespaces."""
31- ip = _ip.IP
32- ip.reset() # first, it should run without error
33+ IP.reset() # first, it should run without error
34 # Then, check that most namespaces end up empty
35- for ns in ip.ns_refs_table:
36- if ns is ip.user_ns:
37+ for ns in IP.ns_refs_table:
38+ if ns is IP.user_ns:
39 # The user namespace is reset with some data, so we can't check for
40 # it being empty
41 continue
42 nt.assert_equals(len(ns),0)
43+
44+
45+def test_user_setup():
46+ """make sure that user_setup can be run re-entrantly in 'install' mode.
47+ """
48+ # This should basically run without errors or output.
49+ IP.user_setup(ip.options.ipythondir,'','install')

Subscribers

People subscribed via source and target branches