attaching files which have spaces in absolute path name fails on second run

[ppurka]

It is not possible to watch a file which has spaces in its absolute path. This is an example:

sage: f1 = 'a.sage'; open(f1,'w').write("print 'h'")             
sage: attach a.sage   
h
sage: attach
attach          attached_files  
sage: attached_files()
['/tmp/a b/a.sage']
sage: open(f1,'w').write("print 'p'")
sage: 
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)

/tmp/a b/<ipython console> in <module>()

/home/punarbasu/Installations/sage-5.0.beta2/local/lib/python2.7/site-packages/sage/misc/preparser.pyc in load(filename, globals, attach)
   1601         if len(v) > 1:
   1602             for file in v:
-> 1603                 load(file, globals, attach=attach)
   1604             return
   1605 

/home/punarbasu/Installations/sage-5.0.beta2/local/lib/python2.7/site-packages/sage/misc/preparser.pyc in load(filename, globals, attach)
   1616 
   1617     if not is_loadable_filename(filename):
-> 1618         raise ValueError('argument (=%r) to load or attach must have extension py, pyx, sage, spyx, or m' % filename)
   1619 
   1620     fpath = os.path.expanduser(filename)

ValueError: argument (='/tmp/a') to load or attach must have extension py, pyx, sage, spyx, or m

---

Apply trac_12671-fix-load-file-with-space-in-path.patch to devel/sage.

[ppurka]

Apply to devel/sage

[ddrake]

I propose we deprecate the support for multiple filenames separated by spaces. The documentation of the function says nothing about multiple files, and since filenames can have spaces, it's impossible or very difficult to properly support a space-separated list of strings that can themselves have spaces!

If we want multiple files, I say the user should give a list.

[ddrake]

Note that #12719 (the IPython 0.12 upgrade) touches the load() function, although their work will not, I think, affect what we do here.

[was]

LGTM

[ddrake]

I strongly disagree with the positive review. The existing code is totally crazy -- it distinguishes between strings with and without spaces by running eval() on them and catching an exception. The patch here does not fix this; it only distinguishes between "a string possibly with spaces that represents a filename" and "a space-separated list of filenames, none of which have spaces".

Keshav Kini and I discussed this and agree that the current design is extremely poor. I am almost done with a simple patch that vastly improves the function.

[was]

ddrake -- can you change the title of the trac ticket to reflect what you now plan to accomplish?

I strongly disagree with the positive review. The existing code is totally crazy -- it distinguishes between strings with and without spaces by running eval() on them and catching an exception.

Thanks for calling my code "totally crazy". I think you are mischaracterizing what the code does. The point of the eval is not to "distinguish between strings with and without spaces". The eval allows for the filename to be determined by valid Python expressions, the motivating case being

   load DATA+"file.sage"

which is commonly used in the notebook. It was never meant to be used with paths that have spaces.

[ddrake]

Replying to was:

I think you are mischaracterizing what the code does. The point of the eval is not to "distinguish between strings with and without spaces". The eval allows for the filename to be determined by valid Python expressions, the motivating case being

   load DATA+"file.sage"

It's possible I'm mischaracterizing...but my understanding of Python function call semantics is that by the time you enter this function, "filename" has already been evaluated. Otherwise, I'm not sure how this could work in the interpreter:

sage: def f(s):
....:     print 'the last character is {0}'.format(s[-1])
....:  
sage: s = 'def'                                                                                                             
sage: f('abc' + s)
the last character is f

The function f does not see "'abc' + s"; it sees the result of evaluating 'abc' + s. The function doesn't need eval(), and so I didn't think that load or attach would either.

Would you prefer that we restore the positive review here and merge this, then, after #12719 is merged, deprecate the keyword versions of load and attach and make them functions that accept either: (1) a string, which must be the name of one file, or (2) a list/tuple/etc of strings?

[was]

Replying to ddrake:

Replying to was:

I think you are mischaracterizing what the code does. The point of the eval is not to "distinguish between strings with and without spaces". The eval allows for the filename to be determined by valid Python expressions, the motivating case being

   load DATA+"file.sage"

It's possible I'm mischaracterizing...but my understanding of Python function call semantics is that by the time you enter this function, "filename" has already been evaluated.

Your confused about what's going on. If you put

    print "filename = '%s'"%filename

at the top of the load function, then do sage -br and type

sage: load "f" + "oo.py"

you'll see

filename = '"f" + "oo.py"'

which illustrates that the input is not eval'd. That's why the docstring starts with

    Executes a file in the scope given by ``globals``.  The
    ``filename`` itself is also evaluated in the scope.

This is documented behavior.

[was]

Replying to ddrake:

Would you prefer that we restore the positive review here and merge this, then, after #12719 is merged, deprecate the keyword versions of load and attach and make them functions that accept either: (1) a string, which must be the name of one file, or (2) a list/tuple/etc of strings?

That sounds like a reasonable plan to me.