Opened 7 years ago

Closed 7 years ago

#12671 closed defect (fixed)

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

Reported by: ppurka Owned by: jason
Priority: major Milestone: sage-5.1
Component: misc Keywords: attach load preparser sd40.5
Cc: Merged in: sage-5.1.beta4
Authors: Punarbasu Purkayastha Reviewers: William Stein
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by 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.

Attachments (1)

trac_12671-fix-load-file-with-space-in-path.patch (1.9 KB) - added by ppurka 7 years ago.
Apply to devel/sage

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by ppurka

  • Authors set to Punarbasu Purkayastha
  • Description modified (diff)
  • Status changed from new to needs_review

Changed 7 years ago by ppurka

Apply to devel/sage

comment:2 Changed 7 years ago by ppurka

  • Keywords sd40.5 added

comment:3 Changed 7 years ago by 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.

comment:4 Changed 7 years ago by 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.

comment:5 Changed 7 years ago by was

  • Status changed from needs_review to positive_review

LGTM

comment:6 Changed 7 years ago by ddrake

  • Status changed from positive_review to needs_work

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.

comment:7 follow-up: Changed 7 years ago by 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.

comment:8 in reply to: ↑ 7 ; follow-ups: Changed 7 years ago by 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?

comment:9 in reply to: ↑ 8 Changed 7 years ago by 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.

comment:10 in reply to: ↑ 8 Changed 7 years ago by 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.

comment:11 Changed 7 years ago by ddrake

  • Reviewers set to William Stein
  • Status changed from needs_work to positive_review

comment:12 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.1.beta4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.