Drop fakemodule #3622
Drop fakemodule #3622
Conversation
| @@ -209,7 +208,8 @@ def store(self, parameter_s=''): | |||
| raise UsageError("Unknown variable '%s'" % args[0]) | |||
|
|
|||
| else: | |||
| if isinstance(inspect.getmodule(obj), FakeModule): | |||
| modname = getattr(inspect.getmodule(obj), '__name__', '') | |||
| if modname == '__main__': | |||
There was a problem hiding this comment.
This also fixes a minor bug with %store - it should print an error if you try to %store a class/instance/function defined in the interactive namespace. But the implementation was based on us using FakeModule for the interactive namespace, which we weren't since I reorganised the interactive namespace machinery some time ago.
This isn't perfect, since if you %run subdir/a.py, things defined in that script will have the module name a, which isn't importable, but it won't warn for those. We could mark modules created for %run with some extra attribute that %store checks, but I'm not sure that it's necessary.
|
It's late and I have to catch a flight in the morning, so I can't really review this in detail. But I want to record a huge +1 from me on this idea: However, I remember introducing it because certain behaviors with So please double, triple-check every imaginable scope/pickling/etc issue you can think of that could be affected by this... I'm almost thinking we should defer this to post 1.0 just to be safe, since it's more of a cleanup than a burning necessity... I just don't want to get hit by an obscure but nasty bug introduced by this that would force us into a rapid 1.0.1.... |
and do this by adding tests to the test suite, naturally... :) |
|
Thanks. I'm happy to leave it until after 1.0, then. I'll do my best to try bizarre corner cases with this. |
|
👍 to putting this in the "immediately post-1.0" stack. |
|
This need a rebase. |
|
Done, it was just a docs conflict. |
|
:-( Need rebase again |
Does this means that now it has been rebased it should be merged ? (not that I'm alway +1 on merging soon to test) |
|
I've added a couple more tests, for the |
| # Since '%run foo' emulates 'python foo.py' at the cmd line, we must | ||
| # set the __file__ global in the script's namespace | ||
| # TK: Is this necessary in interactive mode? | ||
| prog_ns['__file__'] = filename |
There was a problem hiding this comment.
I think matching plain Python is a good idea - interactive Python has no file, whereas script or execfile defines it.
Drop fakemodule After PR #3555, I don't think we have any need for our FakeModule class, so this removes it in favour of just using ModuleType. There shouldn't be any user-visible benefit, so we could leave this until after release if we're going into stability mode. On the other hand, it changes an (undocumented) API, so we might prefer to do this before 1.0.
Drop fakemodule After PR ipython#3555, I don't think we have any need for our FakeModule class, so this removes it in favour of just using ModuleType. There shouldn't be any user-visible benefit, so we could leave this until after release if we're going into stability mode. On the other hand, it changes an (undocumented) API, so we might prefer to do this before 1.0.
[ not for 1.0 ]
After PR #3555, I don't think we have any need for our FakeModule class, so this removes it in favour of just using ModuleType.
There shouldn't be any user-visible benefit, so we could leave this until after release if we're going into stability mode. On the other hand, it changes an (undocumented) API, so we might prefer to do this before 1.0.