Execfile #896

Merged
merged 4 commits into from Oct 19, 2011

Projects

None yet

3 participants

@jstenar
Member
jstenar commented Oct 18, 2011

Work related to #818

Jörgen Stena... added some commits Sep 22, 2011
Jörgen Stenarson fix for execfile that does not tolerate non-ascii characters in filen…
…ame.
26e9691
Jörgen Stenarson Fix for 2.x execfile that can not handle non-ascii filenames.
Special case fix for windows where we can not in general use
sys.getfilesystemencoding() to get a str that can be used to access
all files.
1b786a0
@fperez fperez commented on the diff Oct 18, 2011
IPython/utils/py3compat.py
+ loc = loc if (loc is not None) else glob
+ scripttext = file(fname).read()
+ #compile converts unicode filename to str assuming
+ #ascii. Let's do the conversion before calling compile
+ if isinstance(fname, unicode):
+ filename = unicode_to_str(fname)
+ else:
+ filename = fname
+ exec compile(scripttext, filename, 'exec') in glob, loc
+ else:
+ def execfile(fname, glob, loc=None):
+ if isinstance(fname, unicode):
+ filename = fname.encode(sys.getfilesystemencoding())
+ else:
+ filename = fname
+ __builtin__.execfile(filename, glob, loc)
@fperez
fperez Oct 18, 2011 Member

I thought py3 had removed execfile from the language... Did you actually test this on py3/non-windows? Does it work?

@jstenar
jstenar Oct 18, 2011 Member

Fernando Perez skrev 2011-10-18 21:55:

  •        loc = loc if (loc is not None) else glob
    
  •        scripttext = file(fname).read()
    
  •        #compile converts unicode filename to str assuming
    
  •        #ascii. Let's do the conversion before calling compile
    
  •        if isinstance(fname, unicode):
    
  •            filename = unicode_to_str(fname)
    
  •        else:
    
  •            filename = fname
    
  •        exec compile(scripttext, filename, 'exec') in glob, loc
    
  • else:
  •    def execfile(fname, glob, loc=None):
    
  •        if isinstance(fname, unicode):
    
  •            filename = fname.encode(sys.getfilesystemencoding())
    
  •        else:
    
  •            filename = fname
    
  •        **builtin**.execfile(filename, glob, loc)
    

I thought py3 had removed execfile from the language... Did you actually test this on py3/non-windows? Does it work?

Py3 version is created around line 71 (I have not touched that part).

@fperez
fperez Oct 18, 2011 Member

On Tue, Oct 18, 2011 at 1:03 PM, Jörgen Stenarson
reply@reply.github.com
wrote:

Py3 version is created around line 71 (I have not touched that part).

Ah, OK; no problem then.

@fperez fperez and 1 other commented on an outdated diff Oct 18, 2011
IPython/utils/py3compat.py
@@ -149,3 +151,22 @@ else:
Accepts a string or a function, so it can be used as a decorator."""
return s.format(u='u')
+
+ if sys.platform == 'win32':
+ def execfile(fname, glob, loc=None):
+ loc = loc if (loc is not None) else glob
+ scripttext = file(fname).read()
@fperez
fperez Oct 18, 2011 Member

Should use open instead of file, it's the recommended form.

See http://stackoverflow.com/questions/112970/python-when-to-use-file-vs-open for details.

@jstenar
jstenar Oct 18, 2011 Member

Fernando Perez skrev 2011-10-18 21:56:

@@ -149,3 +151,22 @@ else:

      Accepts a string or a function, so it can be used as a decorator."""
      return s.format(u='u')
  • if sys.platform == 'win32':
  •    def execfile(fname, glob, loc=None):
    
  •        loc = loc if (loc is not None) else glob
    
  •        scripttext = file(fname).read()
    

Should use open instead of file, it's the recommended form.

Fixed in new push.

@fperez
Member
fperez commented Oct 19, 2011

@jstenar, unfortunately when merged against current master, this PR doesn't pass the test suite. Running iptest -vvs IPython.core gives one failure:

======================================================================
ERROR: Test safe_execfile with non-ascii path
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/fperez/usr/lib/python2.7/site-packages/IPython/core/tests/test_interactiveshell.py", line 222, in test_1
    _ip.shell.safe_execfile(self.fname, raise_exceptions=True)
  File "/home/fperez/usr/lib/python2.7/site-packages/IPython/core/interactiveshell.py", line 2284, in safe_execfile
    py3compat.execfile(fname,*where)
  File "/home/fperez/usr/lib/python2.7/site-packages/IPython/utils/py3compat.py", line 172, in execfile
    __builtin__.execfile(filename, glob, loc)
TypeError: must be dict, not None

----------------------------------------------------------------------
Ran 353 tests in 1.176s

FAILED (KNOWNFAIL=2, errors=1)

Do you see it also on Windows? If not, we may need a bit more work to track it down, but if you see the error on Windows, just go ahead and fix it and we'll continue reviewing. Let us know if you need a hand!

@takluyver
Member

The builtin execfile doesn't use None as the defaults for the namespace arguments. I think we need to use the *where trick to pass it the correct number of arguments.

@fperez
Member
fperez commented Oct 19, 2011

If you see the fix right away, feel free to just add one extra commit on top of @jstenar's work and then merge it with the fix... You can just follow steps 1 and 2 of the PR manual merge instructions (click on the 'i' at the left of the green merge bar), and then git reset --hard 9403023 to unwind the merge. Then apply the fix, commit and merge to master/push.

@fperez
Member
fperez commented Oct 19, 2011

I do the above often with PRs where I see a tiny bit of manual cleanup at the end, which is quicker to do myself than to ask the submitter for another round of edits. I think that if the changes will teach the submitter something valuable of the development process (such as adding tests or docs that were missing) it's better to send it back. Similarly if the needed work is significant. But often there's one last bit of easy cleanup missing, and at that point it's just more work to go back and forth on the PR than to simply do it myself and push.

@takluyver
Member

OK, I'll get on it.

@takluyver takluyver merged commit 9403023 into ipython:master Oct 19, 2011
@takluyver
Member

Done. For reference, the extra change I made is here: 4adf668.

@fperez
Member
fperez commented Oct 19, 2011

Thanks a ton, @takluyver! And also @jstenar, for the original work :)

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