search

anntzer / ipython-autoimport

Automagically import missing modules in IPython.
zlib License
58 stars 5 forks source link

test for prun; fix for prun #6

Closed drorspei closed 4 years ago

drorspei commented 4 years ago

This commit changes the way autoimport installs and uninstalls itself when the special magics are called, i.e. time, timeit, and prun.

Instead of patching the class methods without a concrete instance of ExecutionMagics, we keep a pointer to the already registered bound methods.

I added 2 tests: one to check that the magics work fine, and another to check that autoimport is not the user namespace when using the time magic. The latter was passing before the registering change, but I thought it's a good idea to have it.

anntzer commented 4 years ago

I pushed https://github.com/anntzer/ipython-autoimport/commit/5d43aaf8eb18ac140b7dd8479ef9ea9006143ba8 while you were writing this, can you comment on whether you think one solution is better than the other?

drorspei commented 4 years ago

Ahahaha that's funny ^_^

So yeah, I also thought of the inheritance path. This means changing the instance of the magics, actually almost all the magics. If ever these magics contain state, we'd be screwing with it. That's why I went with keeping a pointer to the original bound method.

I've gone over the class ExecutionMagics in IPython 7.13.0, and there is currently no used state. There is one member variable, default_runner, but I don't see it being set to anything but None in the whole IPython project. So maybe it's ok to go with inheritance for now.

I'll keep a branch for this commit in my fork, and we can revisit it if ever magics get out of whack again.

anntzer commented 4 years ago

I don't actually mind reverting my commit and using yours instead, I'm just not sure which one is the more robust. Let me know what you think before I merge #7 and tangle the commit history even more :)

drorspei commented 4 years ago

I think your method is better. It's way simpler, and it works. I'm afraid that my solution, being more complex, has a higher chance of breaking first.

Also, I looked into the magic registry code, and I take back my statement about changing almost all magics. The registry will only override the 3 defined magics in _PatchedMagics and not everything from the base class. So even if other magics in ExecutionMagics gain state, autoimport will be fine.