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

py3: incorporate a tab completion function from sagenb #24998

Closed
fchapoton opened this issue Mar 17, 2018 · 30 comments
Closed

py3: incorporate a tab completion function from sagenb #24998

fchapoton opened this issue Mar 17, 2018 · 30 comments

Comments

@fchapoton
Copy link
Contributor

to continue cutting the links to sagenb

CC: @embray @jdemeyer

Component: python3

Author: Frédéric Chapoton

Branch/Commit: dc2e6f0

Reviewer: John Palmieri

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

@fchapoton fchapoton added this to the sage-8.2 milestone Mar 17, 2018
@fchapoton
Copy link
Contributor Author

Commit: 9bf9f05

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/24998

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 17, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

cdfe056incorporate one function from sagenb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 17, 2018

Changed commit from 9bf9f05 to cdfe056

@fchapoton
Copy link
Contributor Author

comment:3

and bot is morally green

@fchapoton
Copy link
Contributor Author

comment:4

ping ?

@jhpalmieri
Copy link
Member

comment:5

How about some doctests for completions?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 28, 2018

Changed commit from cdfe056 to 039799b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 28, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

039799badding a doctest

@fchapoton
Copy link
Contributor Author

comment:7

done

@fchapoton
Copy link
Contributor Author

comment:8

and bot is morally green again

@embray
Copy link
Contributor

embray commented Mar 28, 2018

comment:9

Why not go ahead and make this Python 3 compatible as well (currently I think it isn't)? Otherwise +1 from me.

@fchapoton
Copy link
Contributor Author

comment:10

Could you precise in which way it is not py3 compatible ? I do not feel very qualified to make it so, by the way. I am quite lost in what exactly is using unicode and what else is using bytes..

@embray
Copy link
Contributor

embray commented Mar 28, 2018

comment:11

Just try testing it on Python 3, I mean. For example, this will break:

            v += [x for x in __builtins__.keys() if x[:n] == s] 

Actually the line is broken on Python 2 as well--there is no __builtins__.keys(). Maybe it means vars(__builtins__)? I'm not sure though. I haven't read the code that closely.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 28, 2018

Changed commit from 039799b to da202c8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 28, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

da202c8simplify the completion method

@fchapoton
Copy link
Contributor Author

comment:13

Here is a tentative of simplification. Now pyflakes is happy.

@fchapoton
Copy link
Contributor Author

comment:14

and bot is morally green again

@jhpalmieri
Copy link
Member

comment:15

I'm happy with it.

@fchapoton
Copy link
Contributor Author

comment:16

Should I remove the unused option ?

@jhpalmieri
Copy link
Member

comment:17

Would that require deprecation?

@fchapoton
Copy link
Contributor Author

comment:18

No, because we are not removing the original method in sagenb, just making inside sage a minimal version suited for our purposes.

@fchapoton
Copy link
Contributor Author

comment:19

So would you give a positive review in the present state, or should I remove the ignored argument ?

@jhpalmieri
Copy link
Member

comment:20

It seems best to remove the ignored argument, but I don't feel strongly about it.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 4, 2018

Changed commit from da202c8 to dc2e6f0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 4, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

9afe56fMerge branch 'u/chapoton/24998' in 8.2.rc1
dc2e6f0trac 24998 removing ignored argument

@fchapoton
Copy link
Contributor Author

comment:22

The ignored argument has now been removed.

@jhpalmieri
Copy link
Member

comment:23

Okay, looks good.

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@vbraun
Copy link
Member

vbraun commented May 12, 2018

Changed branch from u/chapoton/24998 to dc2e6f0

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