Drop fakemodule #3622

Merged
merged 7 commits into from Sep 18, 2013

Projects

None yet

5 participants

@takluyver
Member

[ 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.

@takluyver takluyver commented on the diff Jul 12, 2013
IPython/extensions/storemagic.py
@@ -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__':
@takluyver
takluyver Jul 12, 2013 Member

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.

@fperez
Member
fperez commented Jul 13, 2013

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: FakeModule has been a constant source of weird and obscure bugs over the years, and if we can get all the behavior we need without it, by all means go for it.

However, I remember introducing it because certain behaviors with %run wouldn't work in the same manner than a plain python script at the shell without this hack. I would like to think that all those reasons are now tested, so that if your changes don't break the test suite we're golden. But that may be wishful thinking...

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....

@ivanov
Member
ivanov commented Jul 13, 2013

So please double, triple-check every imaginable scope/pickling/etc issue you can think of that could be affected by this...

and do this by adding tests to the test suite, naturally... :)

@takluyver
Member

Thanks. I'm happy to leave it until after 1.0, then.

I'll do my best to try bizarre corner cases with this.

@minrk
Member
minrk commented Jul 15, 2013

👍 to putting this in the "immediately post-1.0" stack.

@Carreau
Member
Carreau commented Aug 10, 2013

This need a rebase.

@takluyver
Member

Done, it was just a docs conflict.

@Carreau
Member
Carreau commented Aug 16, 2013

:-( Need rebase again

@Carreau
Member
Carreau commented Aug 19, 2013

... to putting this in the "immediately post-1.0" stack.

Does this means that now it has been rebased it should be merged ?

(not that I'm alway +1 on merging soon to test)

@takluyver
Member

I've added a couple more tests, for the -n and -e options to %run. I think the test suite for run is fairly good now, so this is probably safe to merge.

@minrk minrk commented on the diff Sep 18, 2013
IPython/core/magics/execution.py
@@ -550,6 +550,11 @@ def run(self, parameter_s='', runner=None,
__name__save = self.shell.user_ns['__name__']
prog_ns['__name__'] = '__main__'
main_mod = self.shell.user_module
+
+ # 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
@minrk
minrk Sep 18, 2013 Member

I think matching plain Python is a good idea - interactive Python has no file, whereas script or execfile defines it.

@minrk minrk merged commit 2e41dab into ipython:master Sep 18, 2013

1 check was pending

default The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment