Jedi API for Python REPLs #145

Merged
merged 29 commits into from May 18, 2013

4 participants

@tkf
Collaborator

I just noticed that I can create fake scopes from "raw" Python objects. This is just a quick first version of it. Currently it just treats objects with __module__ attribute. I need to treat other cases such as instance and module. I thought I'd post it first before I am going to wrong direction.

Here is a working example:

    >>> from itertools import chain
    >>> script = Interpreter('chain().f', [locals()])
    >>> script.complete()
    [<Completion: <InstanceElement of from_iterable>>]
@tkf
Collaborator

One thing I don't know how to treat: how to complete case like this? (i.e., how to make pr.Name from "raw" builtin Python object?)

>>> x = [{}]
>>> script = Interpreter('x[0].', [locals()])
>>> script.complete()
[..., <Completion: get>, ...]
@davidhalter davidhalter and 1 other commented on an outdated diff Feb 26, 2013
jedi/modules.py
@@ -93,8 +93,10 @@ class ModuleWithCursor(Module):
:param position: The position, the user is currently in. Only important \
for the main file.
"""
- def __init__(self, path, source, position):
+ def __init__(self, path, source, position, fast):
@davidhalter
Owner
davidhalter added a line comment Feb 26, 2013

Why fast? This is being taken care of by the settings.use_fast_parser.

@tkf
Collaborator
tkf added a line comment Feb 26, 2013

Didn't you remove that option? I didn't mess with fast parser at this stage so I am using parsing.Parser in api.Interpreter. api.Script uses fast parser as before. Later, I'll try if I can add scopes to fast parser dynamically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davidhalter
Owner

So, what is the exact goal of this PR? Using it for REPL would be nice, but can you explain a little bit more (not that I need to see immediate results, but I was thinking about REPL some time ago and maybe we could coordinate our efforts.

how to make pr.Name from "raw" builtin Python object?

I'm not exactly sure if that's what you need, but I think you have to use the BuiltinModule. But that's basically for parsing builtin modules. So you probably have to put your names into a container (class).

@tkf
Collaborator

I am thinking about using this in IPython. I don't know if Jedi can be used in IPython codebase, but I guess they will at least provide a way to register completer by extension/plugin. I want this feature because you can't normally complete object in REPL before you instantiate it. So, you can't complete code like Class().f but you can do it using Jedi.

@dbrgn
Collaborator

Ah, that would be very nice!

As soon as we use Jedi from a REPL, we should fix all the __repr__s.

@tkf
Collaborator

Why __repr__? I am not planning to print Jedi classes in REPL. REPL UI should treat how to show the completion candidates.

@dbrgn
Collaborator

Ah, true. I was more talking about playing around with Jedi on a REPL.

@davidhalter
Owner

@tkf
I thought about this two. It could also be an option for the normal python REPL (which I'm still using, because I'm too lazy to type ipython :-)). However I'm not sure that this is the way to go. Because Jedi needs Python code.

Maybe we should look up the type of the object and then try to match it with the existing code, like this:

from os.import join
join().

Now, I'm sure IPython cannot complete this. But I'm not sure if your idea could do this. Because the builtin parser depends on docstrings (which are not always available). In this example I would rather use join.__module__ to detect the original module. After that you could just search for posixpath and then you would be fine.

Do you know what I mean?

As soon as we use Jedi from a REPL, we should fix all the __repr__s.

I don't think they are really broken. Maybe somewhere. But basically we fixed a bug at one place. This is the probably one of the few places get_code is being used in a repr.

@tkf
Collaborator

I would rather use join.__module__ to detect the original module.

This is what I am doing, no? Also, this patch dynamically creates pr.Import instance corresponding to from os.path import join and append it to scope. This way, you don't need to construct a huge str and then parse it.

@dbrgn
Collaborator

I don't think they are really broken. Maybe somewhere. But basically we fixed a bug at one place. This is the probably one of the few places get_code is being used in a repr.

No, I saw a few more. But doesn't matter for now.

@tkf
Collaborator
tkf commented Mar 3, 2013

OK, I implemented the cases I initially planned. api.Interpreter can treat the following types of interpreted Python objects:

Functions and classes:

>>> from os.path import join
>>> join().<COMPLETE>

Modules:

>>> import os
>>> os.path.join().<COMPLETE>

Instances:

>>> import datetime
>>> dt = datetime.datetime(2013, 1, 1)
>>> (dt - dt).<COMPLETE>

I think it is good to merge. There is bit of redundancy because I am not 100% sure I am using right interface. If you want to refactor it bit more before merging it please tell me.

I also tried to use the fast parser but I couldn't because I can't access to self._parser.scope. But I guess there is no need to use the fast parser for a few lines of code (or is it used also for analysing imported modules?). I suggest making it configurable to use the fast parser for api.Script but always use the plain parser for api.Interpreter.

@tkf
Collaborator

Rebased.

@davidhalter
Owner

I might need some time here. I really want this in the core, but it's an API change and I have also some ideas how this might look. So, don't be mad if I'm not merging this in the next few days.

~ david

@tkf
Collaborator

Please take your time. But the API change introduced by this PR is a very small addition. The difference between Script and Interpreter is just the namespaces argument of __init__. I think the change is pretty straight forward.

For internal stuff I admit there could be many improvements. I have some ideas which require internal changes. So I'd like to think about them once we have its frontends/plugins.

@davidhalter
Owner

Yeah... We should also try to refactor some parts of the api.py into other modules (no changes to the API itself). It's grown quite a bit and it's become really confusing to work with it, because of the size.

@tkf
Collaborator
tkf commented May 4, 2013

Rebased.

@coveralls

Coverage Status

Coverage remained the same when pulling 89edb73 on tkf:interpreter-api into ae0dd1d on davidhalter:dev.

@coveralls

Coverage Status

Coverage remained the same when pulling 17f5b9a on tkf:interpreter-api into ae0dd1d on davidhalter:dev.

@davidhalter
Owner

Sorry that this takes so long. I'm really thinking a lot about this. I just realized in the past that pushing out API's is a very dangerous thing. Therefore I'm really reluctant with new API stuff.

However, I like this proposal now much more than when first I looked at it. One thing that I would change is namespaces to namespace. I haven't found a use case where you would need multiple namespaces.

I have to look a little bit into the implementation. Then there are a few smaller things that we might need to discuss. But I don't think they are important right now.

I'm really in favor of this or something like it, and I like the api like this: jedi.Interpreter('chain().f', locals()). So easy!

@tkf
Collaborator
tkf commented May 8, 2013

Thanks for your review!

namespaces takes a list because I thought you might want to pass globals() and locals(), for example.

@tkf
Collaborator
tkf commented May 9, 2013

BTW, once we merge this I think we can drop _quick_complete(source) because it is equivalent to Interpreter(source).completions(). Well, _quick_complete is still shorter but at least you can do it using one line of "public" API.

@dbrgn
Collaborator

Could this be used with the standard REPL? http://hg.python.org/cpython/rev/d5ef330bac50

@tkf
Collaborator
tkf commented May 9, 2013

Yes :)

import sys
import jedi


def jedi_complete(text, state):
    ns = vars(sys.modules['__main__'])
    completions = jedi.Interpreter(text, [ns]).completions()
    try:
        return text + completions[state].complete
    except IndexError:
        return None


try:
    import readline
except ImportError:
    print("Module readline not available.")
else:
    readline.set_completer(jedi_complete)
    readline.parse_and_bind("tab: complete")

Save the above as startup.py and then PYTHONSTARTUP=startup.py python

@dbrgn
Collaborator

Awesome! :) We should put that in the recipes section of the docs once it's in master

@tkf
Collaborator
tkf commented May 9, 2013

Or probably we should add jedi.setup_readline(), so that user can just add it to PYTHONSTARTUP rather than the above one?

@tkf
Collaborator
tkf commented May 9, 2013

Hmm... Sadly, readline does not pass whole text to the completer function. If you type os.path.join().sp<TAB>, it just passes .sp. I guess you need to tweak options in it. See: http://docs.python.org/2/library/readline.html

@tkf tkf referenced this pull request in tkf/emacs-jedi May 9, 2013
Closed

auto complete from *Python* buffer? #51

@davidhalter
Owner

I think we can drop _quick_complete(source)

Yes I also thought of that. But probably we just make the column / line in Script optional. Would be more consistent, wouldn't it?

@tkf
Collaborator

I agree.

@davidhalter
Owner

Alright. I start to really like this proposal. A few things need to be changed however:

  1. As discussed above, drop _quick_complete(source) and add default parameters for Script.
  2. I don't like what we've done to api.py so far and it's getting worse with this PR. Could we move some of that code (all the methods in Interpreter that start with _) to a different place (this place could be e.g. builtin.py, because there we already generate code, but it doesn't have to be, you could also create a new file).
  3. I still don't see a reason to modify modules.py. Could just remove that stuff and test if everything still works?
  4. Speed/Memory limitations. We have to be careful with huge arrays and similar stuff. That's one of the reason why this feature will probably not appear in the next release (but maybe 2 weeks after that).
  5. That still leaves me with the task to look through the code more precisely. But I don't care about that too much now, because we can always change that code (changing the main API is a lot more difficult).

I have also one question left that concerns the main usage of this PR. IMHO the first thing that will happen with this PR is the integration of the python shell and ipython shell. What happens with functions/classes that we start to write in either of these shells? Would your jedi_complete function be enough, especially for ipython?

In the future I would like to see us create a few helper functions in jedi.utils. Something like jedi.utils.shell_complete and jedi.utils.ipython_complete. That would be the easiest way for IDE developers to integrate these great tools. I suppose that many IDEs are using those.

I think that's about it. Tell me what you think. Thank you a lot.

@tkf
Collaborator

I did 1 and 2.

@tkf
Collaborator

I add fix for 3. api.Interpreter uses fast parser now. The reason why I though I couldn't use fast parser was that I was accessing _parser.scope which does not exist in the fast parser. Now I am using _parser.user_scope.

For 4/5, this PR is not for optimizing REPL API. You don't want to do that if it is uncertain to be pulled :). The purpose is rather define API and add working implementation. To optimize, I think import statements must be created on-the-fly (and then cached). To do that, I think subclassing the scope class for the "raw" namespace is a way to go.

I don't know what would happens for IPython. They are middle of refactoring/API redesign for completion machinery [1]. Completer using Jedi could be in the official distribution or could be just an extension. I guess IPython dev will ask to distribute it as an extension first, to see how much useful it is.

For dumb Python REPL, how about bundling startup file in Jedi? We can define a little CLI so that user can just add PYTHONSTARTUP=$(python -m jedi) in their .bashrc etc. to use Jedi in normal Python REPL. My idea is to add something like the above setup code in jedi/replstartup.py and jedi/__main__.py which prints the full path to it.

[1] https://github.com/ipython/ipython/wiki/IPEP-11%3A-Tab-Completion-System-Refactor

@davidhalter
Owner

Yes, I like it! Thank you for the detailed answer. I have to look into IPython completion and the normal completion then. Howver, I like the idea of PYTHONSTARTUP=$(python -m jedi). Very simple.

What would jedi/replstartup.py be?

BTW: _parser.scope shouldn't be public. I hope I can remove that one in the future.

@tkf
Collaborator

I added PYTHONSTARTUP stuff as it is simple. Now you can just do

% PYTHONSTARTUP="$(python -m jedi)" python
Python 2.7.2+ (default, Jul 20 2012, 22:15:08)
[GCC 4.6.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.join().split().in
os.path.join().split().index   os.path.join().split().insert

Completer could be improved. For example, it is maybe a good idea to fallback to rlcompleter.Completer.complete when no completion is found.
http://docs.python.org/2/library/rlcompleter.html

@tkf
Collaborator

I found that completion is slow for some cases, like: os.<TAB>

I guess we need to optimize it a lot (or we can launch Jedi in a separate process, though it sounds like it should be in a separate project).

@davidhalter
Owner

rlcompleter.Completer.complete

Why don't we do it the other way arround, makes more sense IMHO.

@davidhalter
Owner

@tkf One more question: Couldn't we move the __main__.py file into __init__.py and do something like:

if __name__ == '__main__':
    from os import path
    print(path.join(path.dirname(path.abspath(__file__)), 'replstartup.py'))
else:
    [..]

But I have no idea if that would work. I never actually worked with __main__.py files...

So that would be the last question before merging.

@tkf
Collaborator

It looks like you need __main__.py to make package executable, so if __name__ ... works only for module.

You can check that with the following patch.

diff --git a/jedi/__init__.py b/jedi/__init__.py
index 83b69ef..c9e1959 100644
--- a/jedi/__init__.py
+++ b/jedi/__init__.py
@@ -44,3 +44,8 @@ from .api import Script, Interpreter, NotFoundError, set_debug_function
 from . import settings

 sys.path.pop(0)
+
+
+if __name__ == '__main__':
+    from os import path
+    print(path.join(path.dirname(path.abspath(__file__)), 'replstartup.py'))
diff --git a/jedi/__main__.py b/jedi/__main__.py
deleted file mode 100644
index 73cde22..0000000
--- a/jedi/__main__.py
+++ /dev/null
@@ -1,2 +0,0 @@
-from os import path
-print(path.join(path.dirname(path.abspath(__file__)), 'replstartup.py'))

And yes, trying rlcompleter.Completer.complete first then fallback to Jedi makes sense.

@davidhalter
Owner

All right. I think that's good for merge :-) You can do it yourself (including merge).

@tkf tkf added a commit that referenced this pull request May 18, 2013
@tkf tkf Merge branch 'interpreter-api' into dev
Conflicts:
	jedi/__init__.py
	test/test_regression.py

See: #145
5f2477d
@tkf tkf merged commit 3d6ef88 into davidhalter:dev May 18, 2013
@tkf
Collaborator

Merged & pushed. Thanks for your review!

@tkf tkf deleted the tkf:interpreter-api branch May 18, 2013
@davidhalter davidhalter commented on the diff May 24, 2013
jedi/__main__.py
@@ -0,0 +1,2 @@
+from os import path
+print(path.join(path.dirname(path.abspath(__file__)), 'replstartup.py'))
@davidhalter
Owner
davidhalter added a line comment May 24, 2013

@tkf I just wanted to test the repl feature. But: It just prints a path right now (python -m jedi). How can we fix this?

I really want to push this branch as soon as possible.

@tkf
Collaborator
tkf added a line comment May 24, 2013
@davidhalter
Owner
davidhalter added a line comment May 24, 2013

Well I know, but: This line just prints something. Shouldn't it do something?

@tkf
Collaborator
tkf added a line comment May 24, 2013

Ah, I get it. No, it just have to print a path. This part of code is to just to specify a path for PYTHONSTARTUP http://docs.python.org/2/using/cmdline.html#envvar-PYTHONSTARTUP

So, there is actually two Python processes involving in. One is to print the path for PYTHONSTARTUP (i.e., the one runs jedi.__main__). The other one is the REPL and this is where actually jedi is needed (jedi.replstartup properly setups jedi for REPL).

@tkf
Collaborator
tkf added a line comment May 24, 2013

Simply put,

PYTHONSTARTUP=$(python -m jedi)  # runs jedi/__main__.py
python                           # runs jedi/replstartup.py
>>> # hitting TAB here invokes jedi completer
@davidhalter
Owner
davidhalter added a line comment May 24, 2013

Please look at __main__.py again:

https://github.com/davidhalter/jedi/blob/dev/jedi/__main__.py

There's only print there!!!

@tkf
Collaborator
tkf added a line comment May 24, 2013

I said that's fine.

You don't even need __main__.py if you know where jedi is. You can just do something like export PYTHONSTARTUP=/usr/lib/python2.7/site-packages/jedi/replstartup.py in your .bashrc (or whatever). Then just running python starts REPL with Jedi loaded.

@tkf
Collaborator
tkf added a line comment May 24, 2013

If you expect python -m jedi to start python interpreter, that's not the case. Jedi is loaded via $PYTHONSTARTUP mechanism.

@davidhalter
Owner
davidhalter added a line comment Jun 19, 2013

Hmm all right. I still think it would be nice to start the Jedi REPL with python -m jedi.

But that's not a huge issue. We can still do that later.

BTW: The reason why I haven't "pushed" this PR harder in the last few weeks is that it just didn't work out for me as I was expecting. I have to check a few things first before pushing this.

@tkf
Collaborator
tkf added a line comment Jun 19, 2013

If you plan to add something more to python -m jedi, I think it is better to add subcommand CLI (like git) at this point, to avoid incompatible change. But if we want CLI then probably it should go to bin/.

That being said, I don't like the idea Jedi providing a REPL. It should be a plugin, not an entry point. If you want a better REPL, use IPython. I think developing a new REPL at this point makes no sense. Having replstartup.py is an OK solution as it can also act as an minimal example for using Jedi from REPL.

What do you mean by "push"? Pushing to master or releasing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davidhalter
Owner

to avoid incompatible change

what do you mean by that?

It should be a plugin, not an entry point

Yes definitely. I don't mean to create another REPL. But I would like to have an easy entry point. So my goal would be to make some kind of weird hack on python -m jedi to start the REPL with replstartup.py.

One thing I still don't get is why we have __main__.py, when there's no real use (IMHO). But maybe I'm still missing something.

What do you mean by "push"? Pushing to master or releasing?

releasing.

@tkf
Collaborator

to avoid incompatible change

what do you mean by that?

Let's say Jedi user put export PYTHONSTARTUP=$(python -m jedi) in .bashrc. We change python -m jedi to start a REPL break this user's setting in .bashrc because $PYTHONSTARTUP is not a proper path anymore (and he will see an error message every time he starts a shell). This is backward incompatible change.

So my goal would be to make some kind of weird hack on python -m jedi to start the REPL with replstartup.py.

I guess this can be done without a hack. But I think using PYTHONSTARTUP is the standard way to setup completion: http://docs.python.org/2/library/rlcompleter.html

One thing I still don't get is why we have __main__.py, when there's no real use

Do you understand how PYTHONSTARTUP work (http://docs.python.org/2/using/cmdline.html#envvar-PYTHONSTARTUP)? Without __main__.py, how can user know where is the startup file? __main__.py is executed when you open bash via .bashrc (or other file in other shell). This is a real use, no?

@tkf
Collaborator

Let me try explaining again. For simplicity I am assuming you use bash.

To setup Jedi in REPL, you want to execute replstartup.py when you start normal python REPL by python command. Python executes a file at path specified by $PYTHONSTARTUP environment variable, if the file exists. So, next thing you want is to put proper path to $PYTHONSTARTUP everytime you start bash. This is when __main__.py come in. If you run python -m jedi in bash, it prints the path to replstartup.py. So, you can use it to put the path in $PYTHONSTARTUP.

@davidhalter
Owner

Thanks for the explanation. I understand it now, I think. At least theoretically.

The problem is now a practical one: When I delete ~/.pythonrc python complains:

david@...:~$ python
Python 2.7.3 (default, Aug  1 2012, 05:14:39) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
Could not open PYTHONSTARTUP
IOError: [Errno 2] No such file or directory: '/home/david/.pythonrc'
>>> 

Now when I create it (empty), completion doesn't work anymore (at all). Obviously it works when I set up completion by myself (creating a .pythonrc file & calling setup_readline). However, it doesn't work at all when I don't do that. I would expect having export PYTHONSTARTUP="$(python -m jedi)" in my .bashrc would make the whole thing working (according to http://jedi.jedidjah.ch/en/dev/docs/repl.html).

Let me try explaining again. For simplicity I am assuming you use bash.

Yes I use bash. Is there any difference otherwise?

@tkf
Collaborator

I don't think Python cares about .pythonrc by default. In man python it mentions ~/.pythonrc.py, but no .pythonrc.

   ~/.pythonrc.py
          User-specific initialization file loaded by the user module; not
          used by default or by most applications.

My guess is that you (or your OS?) set $PYTHONSTARTUP accidentally somewhere. What do you see by echo $PYTHONSTARTUP when you have this problem?

Yes I use bash. Is there any difference otherwise?

For example, I use zsh. So I should be putting it in .zshrc or somewhere else instead of .bashrc. I could be calling it "shell setting file" but I thought the words were too long.

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