#11235 closed defect (fixed)
Make the ipython edit magic command edit the right file and show both files when doing ??
Reported by:  mderickx  Owned by:  jason 

Priority:  major  Milestone:  sage5.0 
Component:  misc  Keywords:  sd35 ipython source file location 
Cc:  Merged in:  sage5.0.beta2  
Authors:  Maarten Derickx  Reviewers:  Marco Streng 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
All is really said in the title.
apply:
and don't forget the spkg at http://boxen.math.washington.edu/home/jdemeyer/spkg/ipython0.10.2.p1.spkg
Attachments (4)
Change History (38)
comment:1 Changed 9 years ago by
Changed 9 years ago by
comment:2 Changed 9 years ago by
 Status changed from new to needs_review
comment:3 Changed 9 years ago by
 Description modified (diff)
comment:4 Changed 9 years ago by
Have you looked at sage/misc/edit_module.py? Would it be better to plug into that functionality when you type "%edit"?
comment:5 Changed 9 years ago by
Yeah I looked at it, but I think that following the arguments of least surprise it would be good to just keep the IPython behavior as much as possible . People might already know the exact behaviour of %edit because they have used IPython. And since typing % in front of something is really an IPython thing, so I think we should keep their implementation and not our custom one (at least when doing %edit, you can always use the edit() function if you want the sage behaviour.
comment:6 Changed 9 years ago by
Note that before the patch %edit also worked, but it opened the wrong file, all that the patch does is change the file it opens.
comment:7 Changed 9 years ago by
comment:9 Changed 8 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
I added a new spkg http://www.math.leidenuniv.nl/~mderickx/ipython0.10.2.p1.spkg
comment:10 followup: ↓ 11 Changed 8 years ago by
09:56:43:~$ tar xf ipython0.10.2.p1.spkg ipython0.10.2.p1/src/docs/examples/kernel/davinci2.txt: Truncated tar archive tar: Error exit delayed from previous errors.
???
comment:11 in reply to: ↑ 10 Changed 8 years ago by
Replying to mstreng:
09:56:43:~$ tar xf ipython0.10.2.p1.spkg ipython0.10.2.p1/src/docs/examples/kernel/davinci2.txt: Truncated tar archive tar: Error exit delayed from previous errors.???
Never mind that, I downloaded a partial file apparently. Got the right one now.
comment:12 Changed 8 years ago by
 Keywords sd35 ipython source file location added
Which Sage version did you test this on? I installed the spkg, then the patch, and the patch gave me:
10:10:22:~/sage4.8.alpha4/devel/sage$ hg qpush applying IPythonedit.patch patching file sage/misc/edit_module.py Hunk #1 FAILED at 39 1 out of 3 hunks FAILED  saving rejects to file sage/misc/edit_module.py.rej patch failed, unable to continue (try v) patch failed, rejects left in working dir errors during apply, please fix and refresh IPythonedit.patch 10:10:54:~/sage4.8.alpha4/devel/sage$ cat sage/misc/edit_module.py.rej  edit_module.py +++ edit_module.py @@ 40,6 +40,8 @@ import inspect import os import sys +import re +import IPython from string import Template
comment:13 Changed 8 years ago by
 Reviewers set to Marco Streng
 Status changed from needs_review to needs_work
 Work issues set to rebase on top of 4.8.alpha4
And it is not because I installed the spkg first: on a Sage 4.8.alpha4 without the spkg, I get the same problem.
Changed 8 years ago by
comment:14 Changed 8 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
Ok I updated the patch.
comment:15 Changed 8 years ago by
 Status changed from needs_review to needs_work
Sorry I missed the trac update. I'm reviewing right now. Like it very much.
Mild reasons for needs_work:
 incorrect spelling of diffrent
 I guess you can't doctest
edit_devel
, but it would be good to get rid of the following warning:sage coverage devel/sage/sage/misc/edit_module.py  devel/sage/sage/misc/edit_module.py SCORE devel/sage/sage/misc/edit_module.py: 100% (6 of 6) Possibly wrong (function name doesn't occur in doctests): * edit_devel(self, filename, linenum): 
Suggestions for the patch (these things aren't regressions, they were like this always, so if you don't want to spend time on it, that's fine with me):
 After %ed, the file is reloaded, which is not the correct thing to do. It should be imported, otherwise doing %ed doesn't make sense. Example:
sage: EllipticCurve([1,2,3,4,6]) Elliptic Curve defined by y^2 + x*y + 3*y = x^3 + 2*x^2 + 4*x + 6 over Rational Field sage: %ed EllipticCurve Editing... done. Executing edited code... sage: EllipticCurve([1,2,3,4,6]) ImportError: No module named ell_generic
 when the
.sub
is successful andfilename
ends with".sage"
, it would be easy to afterwards overwrite the sitepackages file with the source file. Then the user only has tosage: from sage.all import *
(which you can tell the user to do, or maybe program Sage to do)
comment:16 Changed 8 years ago by
There are no changes to the notebook, right?
comment:17 Changed 8 years ago by
 Work issues changed from rebase on top of 4.8.alpha4 to typo and doctest coverage warning
(all tests pass on 4.8.alpha4)
comment:18 followup: ↓ 19 Changed 8 years ago by
 Work issues typo and doctest coverage warning deleted
I thought I'd fix the typo and put up a reviewer patch, but found out that there is something duplicate about your patch.
The functionality that your patch provides in sage.misc.edit_module.edit_devel
is already in sage.misc.edit_module.edit
. See http://www.sagemath.org/doc/reference/sage/misc/edit_module.html. See also #7961.
On an unpatched sage4.8.alpha4, I get the correct file with sage: edit(EllipticCurve)
, but the incorrect file with sage: %ed EllipticCurve
comment:19 in reply to: ↑ 18 ; followup: ↓ 22 Changed 8 years ago by
Replying to mstreng:
I thought I'd fix the typo and put up a reviewer patch, but found out that there is something duplicate about your patch.
But I guess you know all that. This is about %ed
, not edit
comment:20 Changed 8 years ago by
 Description modified (diff)
comment:21 Changed 8 years ago by
 Status changed from needs_work to needs_review
Positive review for everything but the reviewer patch. Please review the reviewer patch!
comment:22 in reply to: ↑ 19 Changed 8 years ago by
Replying to mstreng:
Replying to mstreng:
I thought I'd fix the typo and put up a reviewer patch, but found out that there is something duplicate about your patch.
But I guess you know all that. This is about
%ed
, notedit
Yes this is exactly the reason. I would like to review your review patch, and at least I agree with the documentation changes. But I'm terrible at spelling so it can have positive review as soon as someone just can guarente to me that the English is correct
comment:23 Changed 8 years ago by
The English in the reviewer patch looks fine to me (although I would have said "the Sage library" instead of "the sage library"). So feel free to switch this to a positive review.
comment:24 Changed 8 years ago by
 Status changed from needs_review to positive_review
comment:25 followup: ↓ 26 Changed 8 years ago by
There is a small problem with the documentation:
/mnt/usb1/scratch/jdemeyer/merger/sage5.0.beta2/local/lib/python2.7/sitepackages/sage/misc/edit_module.py:docstring of sage.misc.edit_module.edit_devel:14: WARNING: Inline interpreted text or phrase reference startstring without endstring.
comment:26 in reply to: ↑ 25 Changed 8 years ago by
Replying to jdemeyer:
What causes this? Is it the ``%edit``
in the docstring?
comment:27 Changed 8 years ago by
No, it's the dollar sign. Try this:

sage/misc/edit_module.py
diff git a/sage/misc/edit_module.py b/sage/misc/edit_module.py
a b def edit_devel(self, filename, linenum): 299 299 sage: %ed gcd # indirect doctest, not tested 300 300 301 301 The above should open your favorite editor (as stored in the environment 302 variable $EDITOR) with the file in which gcd is defined, and when your302 variable \$EDITOR) with the file in which gcd is defined, and when your 303 303 editor supports it, also at the line in wich gcd is defined. 304 304 """ 305 305 sageroot = sage.misc.sageinspect.SAGE_ROOT+'/'
Changed 8 years ago by
comment:28 Changed 8 years ago by
 Description modified (diff)
 Status changed from positive_review to needs_work
comment:29 Changed 8 years ago by
 Status changed from needs_work to needs_review
comment:30 Changed 8 years ago by
11235_doc.patch needs review.
comment:32 Changed 8 years ago by
 Merged in set to sage5.0.beta2
 Resolution set to fixed
 Status changed from positive_review to closed
comment:33 Changed 8 years ago by
 Description modified (diff)
spkg contains garbage:
jdemeyer@sage:~/spkg/ipython0.10.2.p1$ hg status ? .spkginstall.swp
Fixed at http://boxen.math.washington.edu/home/jdemeyer/spkg/ipython0.10.2.p1.spkg
comment:34 Changed 8 years ago by
The spkg wasn't qfinish
ed, fixed this also.
Here is the spkg which patches IPython http://www.math.leidenuniv.nl/~mderickx/ipython0.9.1.p1.spkg. Since there is no ipython hook you really need the patch here to show both files.