Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Followup to #34547: fix emacs sage-shell-mode #34935

Closed
jhpalmieri opened this issue Jan 24, 2023 · 12 comments
Closed

Followup to #34547: fix emacs sage-shell-mode #34935

jhpalmieri opened this issue Jan 24, 2023 · 12 comments

Comments

@jhpalmieri
Copy link
Member

It turns out that sage-shell-mode in emacs uses the variable interfaces, deleted from sage.interfaces.all in #34547. We restore that variable (which is not used anywhere in the Sage library) along with a comment.

Component: interfaces

Keywords: emacs

Author: John Palmieri

Branch/Commit: u/jhpalmieri/interfaces-emacs @ 49d4ff7

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/34935

@jhpalmieri jhpalmieri added this to the sage-9.8 milestone Jan 24, 2023
@jhpalmieri
Copy link
Member Author

Branch: u/jhpalmieri/interfaces-emacs

@jhpalmieri
Copy link
Member Author

New commits:

8e76d0dtrac 34935: fix sage-shell mode in emacs

@jhpalmieri
Copy link
Member Author

Commit: 8e76d0d

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 24, 2023

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 24, 2023

comment:3

The backslashes are not needed, but positive review nevertheless.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 24, 2023

Changed commit from 8e76d0d to 49d4ff7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 24, 2023

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

49d4ff7trac 34935: fix sage-shell mode in emacs

@jhpalmieri
Copy link
Member Author

comment:5

Right, sorry, I was just copying the old lines back. Let's get rid of the backslashes.

@mantepse
Copy link
Collaborator

comment:6

Wow, that's great!

Now that you found the culprit, what do you think about simply removing the offending lines in sage-shell-mode itself?

I admit that I do not understand at all why names in interfaces get a special treatment.

Another special case I do not understand is line 351:

ignore_classes = [sage.interfaces.gap.Gap, sage.misc.lazy_import.LazyImport]

We could actually take the opportunity do (optionally?) remove the underscore and double underscore methods from the completion list. I never understood that the code was so simple!

I would do this, if at least one further person is interested.

--- /home/martin/.emacs.d/elpa/sage-shell-mode-20221020.1012/emacs_sage_shell.py.old	2023-01-25 07:57:39.845564969 +0100
+++ /home/martin/.emacs.d/elpa/sage-shell-mode-20221020.1012/emacs_sage_shell.py	2023-01-25 07:58:33.123905933 +0100
@@ -52,8 +52,6 @@
 except:
     pass
 
-interfaces = ip.ev('interfaces')
-
 _sage_const_regexp = re.compile("_sage_const_")
 
 
@@ -168,12 +166,10 @@
         regexp = re.compile("^[ a-zA-Z0-9._\\[\\]]+$")
         if regexp.match(varname) is None:
             return []
-        if varname in interfaces:
-            ls = ip.ev('dir(%s)' % (varname,))
-        else:
-            ls = _completions_attributes(preparse(varname))
-            ls.extend(ip.ev('dir(%s)' % (preparse(varname),)))
-            ls = list(sorted(set(ls)))
+        name = preparse(varname)
+        ls = _completions_attributes(name)
+        ls.extend(ip.ev('dir(%s)' % (name,)))
+        ls = list(sorted(set(ls)))
         return ls
     except:
         return []

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Jan 25, 2023

comment:7

Replying to John Palmieri:

Right, sorry, I was just copying the old lines back. Let's get rid of the backslashes.

I confirm that 9.8.beta7 + the present patch runs under sage-shell-mode.

Thanks a lot !

@jhpalmieri
Copy link
Member Author

comment:8

Replying to Martin Rubey:

Wow, that's great!

Now that you found the culprit, what do you think about simply removing the offending lines in sage-shell-mode itself?

I don't use sage-shell-mode, so I don't know what users would lose if we got rid of those lines. I think we should keep this change for now and then possibly change sage-shell-mode separately. If we remove this from sage-shell-mode, then some time after that, we can remove the interfaces variable from Sage. (We probably should keep it for a while for people using older versions of sage-shell-mode.)

mkoeppe pushed a commit that referenced this issue Feb 6, 2023
It turns out that sage-shell-mode in emacs uses the variable
`interfaces`, deleted from `sage.interfaces.all` in #34547. We restore
that variable (which is not used anywhere in the Sage library) along
with a comment.

URL: https://trac.sagemath.org/34935
Reported by: jhpalmieri
Ticket author(s): John Palmieri
Reviewer(s): Matthias Koeppe
@kwankyu
Copy link
Collaborator

kwankyu commented Feb 11, 2023

Merged to Sage 9.8.rc1

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

No branches or pull requests

4 participants