ENH: extensions: port autoreload to current API #746

Merged
merged 5 commits into from Oct 1, 2011

Conversation

Projects
None yet
3 participants
@pv
Contributor

pv commented Aug 30, 2011

A patch to port the autoreload.py extension to use the current API. Tested to work as intended (also in the qtconsole), but I don't exactly know what happens if it's used with parallel execution.

@fperez

This comment has been minimized.

Show comment Hide comment
@fperez

fperez Sep 13, 2011

Owner

@pv, this looks great, many thanks for resurrecting it! The only thing I'd like to ask for is some testing... I know this one is tricky to test, but we should be able to come up with something.

Also, you may want to refactor the functionality so that the magics are just thin wrappers that call self-contained library functions. That will make it much easier to test them, especially if they don't really need the entire ipython instance.

Once we can sort out some form of testing for this, we'll proceed with merging it, because there's no doubt we want the functionality.

Owner

fperez commented Sep 13, 2011

@pv, this looks great, many thanks for resurrecting it! The only thing I'd like to ask for is some testing... I know this one is tricky to test, but we should be able to come up with something.

Also, you may want to refactor the functionality so that the magics are just thin wrappers that call self-contained library functions. That will make it much easier to test them, especially if they don't really need the entire ipython instance.

Once we can sort out some form of testing for this, we'll proceed with merging it, because there's no doubt we want the functionality.

@pv

This comment has been minimized.

Show comment Hide comment
@pv

pv Sep 18, 2011

Contributor

Ok, it's now refactored as you suggested, and I added functional tests for it.

Contributor

pv commented Sep 18, 2011

Ok, it's now refactored as you suggested, and I added functional tests for it.

@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Sep 29, 2011

Owner

@fperez: Shall we go ahead and merge this one?

Owner

takluyver commented Sep 29, 2011

@fperez: Shall we go ahead and merge this one?

@fperez

This comment has been minimized.

Show comment Hide comment
@fperez

fperez Sep 30, 2011

Owner

If you don't see any other issues, +1, thanks! Sorry that I've been a bit swamped locally and fell behind, @pv.

Owner

fperez commented Sep 30, 2011

If you don't see any other issues, +1, thanks! Sorry that I've been a bit swamped locally and fell behind, @pv.

@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Oct 1, 2011

Owner

@pv: It's currently showing up as a test failure, because it recognises the module docstring as a doctest, and then doesn't find foo and some_function(). There's a decorator you can use for functions or methods (IPython.testing.skipdoctest.skip_doctest), but I admit I'm not sure how to do the same for a module.

Owner

takluyver commented Oct 1, 2011

@pv: It's currently showing up as a test failure, because it recognises the module docstring as a doctest, and then doesn't find foo and some_function(). There's a decorator you can use for functions or methods (IPython.testing.skipdoctest.skip_doctest), but I admit I'm not sure how to do the same for a module.

@fperez

This comment has been minimized.

Show comment Hide comment
@fperez

fperez Oct 1, 2011

Owner

No worries @takluyver, it's just a matter of adding skip_doctest = True to the module itself. Did it now, will merge with this small fix. Thanks again, @pv!

Owner

fperez commented Oct 1, 2011

No worries @takluyver, it's just a matter of adding skip_doctest = True to the module itself. Did it now, will merge with this small fix. Thanks again, @pv!

@fperez fperez merged commit 632ea8f into ipython:master Oct 1, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment