Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#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: sage-5.0
Component: misc Keywords: sd35 ipython source file location
Cc: Merged in: sage-5.0.beta2
Authors: Maarten Derickx Reviewers: Marco Streng
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Attachments (4)

IPython-edit.patch (1.5 KB) - added by mderickx 9 years ago.
IPython-edit.patch.patch (1.8 KB) - added by mderickx 8 years ago.
11235-reviewer.patch (1.8 KB) - added by mstreng 8 years ago.
reST formatting, spelling, clarity
11235_doc.patch (2.0 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 9 years ago by mderickx

Here is the spkg which patches IPython http://www.math.leidenuniv.nl/~mderickx/ipython-0.9.1.p1.spkg. Since there is no ipython hook you really need the patch here to show both files.

Changed 9 years ago by mderickx

comment:2 Changed 9 years ago by mderickx

  • Status changed from new to needs_review

comment:3 Changed 9 years ago by mderickx

  • Description modified (diff)

comment:4 Changed 9 years ago by jhpalmieri

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 mderickx

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 mderickx

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 mderickx

  • Authors set to Maarten Derickx

comment:8 Changed 8 years ago by mstreng

  • Status changed from needs_review to needs_work

new ipython

comment:9 Changed 8 years ago by mderickx

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:10 follow-up: Changed 8 years ago by mstreng

09:56:43:~$ tar -xf ipython-0.10.2.p1.spkg 
ipython-0.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 mstreng

Replying to mstreng:

09:56:43:~$ tar -xf ipython-0.10.2.p1.spkg 
ipython-0.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 mstreng

  • 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:~/sage-4.8.alpha4/devel/sage$ hg qpush
applying IPython-edit.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 IPython-edit.patch
10:10:54:~/sage-4.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 mstreng

  • 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 mderickx

comment:14 Changed 8 years ago by mderickx

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Ok I updated the patch.

comment:15 Changed 8 years ago by mstreng

  • 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 and filename ends with ".sage", it would be easy to afterwards overwrite the site-packages file with the source file. Then the user only has to sage: 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 mstreng

There are no changes to the notebook, right?

comment:17 Changed 8 years ago by mstreng

  • 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 follow-up: Changed 8 years ago by mstreng

  • 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 sage-4.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 ; follow-up: Changed 8 years ago by 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, not edit

Changed 8 years ago by mstreng

reST formatting, spelling, clarity

comment:20 Changed 8 years ago by mstreng

  • Description modified (diff)

comment:21 Changed 8 years ago by mstreng

  • 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 mderickx

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, not edit

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 jhpalmieri

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 mderickx

  • Status changed from needs_review to positive_review

comment:25 follow-up: Changed 8 years ago by jdemeyer

There is a small problem with the documentation:

/mnt/usb1/scratch/jdemeyer/merger/sage-5.0.beta2/local/lib/python2.7/site-packages/sage/misc/edit_module.py:docstring of sage.misc.edit_module.edit_devel:14: WARNING: Inline interpreted text or phrase reference start-string without end-string.

comment:26 in reply to: ↑ 25 Changed 8 years ago by mstreng

Replying to jdemeyer:

What causes this? Is it the ``%edit`` in the docstring?

comment:27 Changed 8 years ago by jhpalmieri

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): 
    299299        sage: %ed gcd           # indirect doctest, not tested
    300300
    301301    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 your
     302    variable \$EDITOR) with the file in which gcd is defined, and when your
    303303    editor supports it, also at the line in wich gcd is defined.
    304304    """
    305305    sageroot = sage.misc.sageinspect.SAGE_ROOT+'/'

Changed 8 years ago by jdemeyer

comment:28 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from positive_review to needs_work

comment:29 Changed 8 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:30 Changed 8 years ago by jdemeyer

11235_doc.patch needs review.

comment:31 Changed 8 years ago by mstreng

  • Status changed from needs_review to positive_review

Thanks Jeroen

comment:32 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:33 Changed 8 years ago by jdemeyer

  • Description modified (diff)

spkg contains garbage:

jdemeyer@sage:~/spkg/ipython-0.10.2.p1$ hg status
? .spkg-install.swp

Fixed at http://boxen.math.washington.edu/home/jdemeyer/spkg/ipython-0.10.2.p1.spkg

comment:34 Changed 8 years ago by jdemeyer

The spkg wasn't qfinished, fixed this also.

Note: See TracTickets for help on using tickets.