Add support for relative import to %run -m (fixes #2727)#3066
Conversation
test_run_submodule_with_relative_import fails.
|
This is the test failure: It is from |
There was a problem hiding this comment.
Can this line have a comment to explain when it happens? The comments on the if/elif may also need to be updated.
There was a problem hiding this comment.
On second thoughts, maybe we should refactor this. Currently prun is used both as a magic command, and as a function to be called by other code, which makes it somewhat confusing. Maybe the core should be pulled out into another function, so that prun is just for the magic command. Going by your other changes, the new function could be called _run_with_profiler.
There was a problem hiding this comment.
I thought it would be better to not break backward compatibility. As it is a user facing function (i.e., magic function), someone might be using it for writing their own magic function. But I would refactor and add _run_with_profiler if you think it is OK. I think we can get rid of the else clause as I think it is not used in IPython.
FYI, you can see the parameter spec in the documentation:
http://ipython.org/ipython-doc/dev/api/generated/IPython.core.magics.execution.html#IPython.core.magics.execution.ExecutionMagics.prun
There was a problem hiding this comment.
Backwards compatibility would already have been broken when we refactored the magics into separate classes for 0.13, and I don't remember hearing about any problems, so I don't think it's an issue. We can add a note to the "what's new" document in case anyone is using it.
The API docs unfortunately default to documenting every class and function, without us ever having to consider what constitutes our public API. I've added an @undoc decorator to explicitly mark things as private, but I'm reluctant to apply it to magic classes, because at present the API docs are the only fully comprehensive documentation of the magic commands we offer.
There was a problem hiding this comment.
If I can change it that's good. I just played on safer side. The function is clear now, I think.
|
Definitely +1 on breaking run up into some smaller pieces like this. |
There was a problem hiding this comment.
Can this have a comment explaining that it's filling in the default options for any that aren't specified?
|
Thanks, I think this is looking good. |
There was a problem hiding this comment.
@takluyver This was error(msg) and return. But as _run_with_debugge is a separated function now, return does not finish run. Do you think this is OK? I am not 100% sure that I can replace error(msg) and return with raise UsageError(msg).
There was a problem hiding this comment.
I think magics are supposed to raise a UsageError now, instead of error()ing and returning. It does mean that this code should only be called by a magic function, but since it's a private method in a Magics class, I think that's reasonable.
|
I haven't dug into the details here, but I certainly agree that |
|
This is OK by me now, but as it touches some core and rather fragile stuff, I'd like to have a second set of eyes look over it before we merge it. I think @fperez said he'd have more time soon, so maybe he'll have a look. |
|
We should also update the main |
|
I fixed the usage line. |
|
Looks like this PR has gotten some good review and updates. There was a mention of having @fperez looking at it because he knows this code well. Do you still feel like that is desired? Other than that, is this ready to go? |
|
I'd ideally like a second pair of eyes on it, because the %run stuff is quite delicate. I think @fperez knows it best. But we have got reasonable test coverage for it now, so if Fernando's not going to get to it soon, we could push the green button and deal with any fallout that might occur. |
There was a problem hiding this comment.
Even though it's private, I'd want to see a docstring here... Functions without docstrings are just painful for whomever comes later, as it's very hard to make out the intent of the arguments.
|
Great job, @tkf! Very minor cosmetic issues there, otherwise it looks solid. Honestly, Once the small issues are ironed out, I think we can go ahead and merge. Thanks everyone for the earlier review, and sorry for my slowness! |
|
@fperez I just fixed the points you mentioned. |
Add support for relative import to %run -m (fixes #2727)
Add support for relative import to %run -m (fixes ipython#2727)
5 participants
%run is refactored a little bit by side effect :)