[READY] Use a http wrapper for jedi #246

vheon · 2015-09-27T11:59:59Z

This is the first step to support python completions for python version different from the one ycmd is currently running.

What this change does

The jedi library is still used, but instead of calling it directly we now wrapped in a http+JSON server.

Testing

The test have been updated to solicit the completer first with a FileReadyToParse event, which would start a JediHTTP server if one is not running (similarly to the OmnisharpServer tests). The tests for the GoTo subcommand family have been updated to test all three of the subcommands.

Notes

Two subcommand have been added: RestartServer and StopServer but only the RestartServer is exposed through the user by the DefinedSubcommand method, this is because I think that the StopServer command is not useful for the user but is useful in the testing phase for shutting down the JediHTTP.

I have one question: would make sense to rename any reference from jedi_completer to python_completer? If it is I will make another PR.

@reinhrst If you could test this would be awesome :)

Fixes ycm-core/YouCompleteMe#1827

reinhrst · 2015-09-28T05:54:10Z

Have it running, again with changing the following change (since I really need the python3 support; I fully agree it makes sense for now to not add that to the PR; one step at a time)

--- a/ycmd/completers/python/jedi_completer.py
+++ b/ycmd/completers/python/jedi_completer.py
@@ -31,7 +31,7 @@ import sys
 import os


-PYTHON_EXECUTABLE_PATH = sys.executable
+PYTHON_EXECUTABLE_PATH = "python"

vheon · 2015-09-28T19:51:43Z

@reinhrst thanks for testing this out! So at this point the review is needed :) @Valloric @micbou @puremourning

puremourning · 2015-09-28T20:03:50Z

@vheon you want us to take a look at the jedihttp code itself?

vheon · 2015-09-28T20:06:03Z

if you have the time would be great.

lithammer · 2015-09-28T20:12:57Z

ycmd/completers/python/jedi_completer.py

+    """ Check if JediHTTP server is ready. """
+    try:
+      return bool( self._GetResponse( '/ready' ) )
+    except:


I'm not sure if you're aware or not, but usually you don't want a "bare" except:.

A bare except: clause will catch SystemExit and KeyboardInterrupt exceptions, making it harder to interrupt a program with Control-C, and can disguise other problems. If you want to catch all exceptions that signal program errors, use except Exception:.

https://www.python.org/dev/peps/pep-0008/#programming-recommendations

This applies to a lot of places in this pull request.

No I didn't know, but I did like we did in the cs_completer :S

puremourning · 2015-09-28T20:54:03Z

Of course! No problem :)

Valloric · 2015-09-29T01:08:24Z

@Renstrom I highly recommend making line-level comments on the Reviewable.io review (link in PR description), not on github. :)

vheon · 2015-09-30T21:02:20Z

A question taken from @reinhrst comment in #160 (comment):

Is it secure enough? Although it's running on a random port, should perhaps all requests be protected with a secret, as in the case of ycmd itself?

So the question here is clear, should the new JediHTTP server have a secret shared with ycmd so we are sure that we answer only to ycmd requests?

micbou · 2015-10-01T14:38:27Z

Reviewed 11 of 12 files at r1, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

^{ycmd/completers/python/jedi_completer.py, line 252 [r3] (raw file):}
You can avoid nested try blocks by using the pass keyword in the except part:

try:
    ...
except Exception:
    pass

try:
    ...

^{ycmd/tests/test_utils.py, line 113 [r3] (raw file):}
Remove ; at the end of the line.

Comments from the review on Reviewable.io

Valloric · 2015-10-02T01:41:27Z

@vheon It wouldn't hurt. Getting MAC auth right is tricky though; look at how ycmd does it. That code has been security reviewed by professional security researchers.

BTW sorry for the delay in reviewing this; this + the JediHTTP code itself is pretty big and I don't have a solid chunk of time to look at it yet. I'll try to find time this weekend.

Review status: 10 of 12 files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

vheon · 2015-10-02T20:46:48Z

Do you think is reasonable to add the HMAC feature later as another PR? That way the review will be much simpler and focused.

Anyway don't worry about the delay ;)

Review status: 10 of 12 files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

vheon · 2015-10-02T21:22:52Z

Would be nice If I could use the same hmac_plugin for bottle as ycmd use without basically duplicate the code :S

Review status: 10 of 12 files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

puremourning · 2015-10-04T14:07:06Z

Few idle thoughts:

It would be cool if we could use a unix domain socket, rather than a tcp port where that feature is available. Using host ports can cause conflicts, and the mechanism we use typically suffers from a (rare) race condition. I know that waitress supports it because i have a branch which implements it for YCM-ycmd communication. On systems at my work, TCP ports (even ephemeral ones) are a coveted resource (for long, complex and boring reasons), and random processes listening on tcp ports can cause issues. I know this might be a specific use-case, but worth considering?
I don't know exactly if this is an issue or not, but does the working directory of the jedihttp process matter in any meaningful way? I mean currently if you open a python file in Vim and then :cd to another directory then we invoke jedi, does the working directory of jedi change with Vim? It could be meaningful to things like import directories? What I'm wondering is should/could we pass the Vim working directory to JediHTTP on each request, now that we pass it from Vim client.
I've taken a look at the JediHTTP code, but I couldn't find a simple way to add any line comments. Is there a way to get it into reviewable.io or some equivalent?

Reviewed 10 of 12 files at r1, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

^{ycmd/completers/python/jedi_completer.py, line 100 [r4] (raw file):}
We should probably print the message before actually doing it.

^{ycmd/completers/python/jedi_completer.py, line 117 [r4] (raw file):}
I'm almost certainly wrong here, but doesn't the with open(...) construct call close at the end of the with block? Isn't that the whole point of the with ? My python fu isn't great so just checking :)

^{ycmd/completers/python/jedi_completer.py, line 121 [r4] (raw file):}
Also here, shouldn't we log "Starting JediHTTP server..." prior to running popen? That way any launch errors appear in the more natural sequence in the log file

^{ycmd/completers/python/jedi_completer.py, line 189 [r4] (raw file):}
any reason not to just to

return self._GetResponse( ... )

?

^{ycmd/completers/python/jedi_completer.py, line 194 [r4] (raw file):}
I don't think these parens are necessary

Comments from the review on Reviewable.io

vheon · 2015-10-04T14:56:37Z

Review status: 11 of 12 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

^{ycmd/completers/python/jedi_completer.py, line 117 [r4] (raw file):}
My python is not so fluent so I mimic what we currently do in the omnisharp completer. Anyway this is why we do it like that:

if the Popen fails we automatically close the log files
once the process is open we try to close the files but they are also used by the process itself so I don't think they will be closed.

Any more light on this is welcome though :)

^{ycmd/completers/python/jedi_completer.py, line 189 [r4] (raw file):}
I think is a debug-thing habit :(

Comments from the review on Reviewable.io

vheon · 2015-10-04T15:10:20Z

Few idle thoughts:

It would be cool if we could use a unix domain socket, rather than a tcp port where that feature is available. Using host ports can cause conflicts, and the mechanism we use typically suffers from a (rare) race condition. I know that waitress supports it because i have a branch which implements it for YCM-ycmd communication. On systems at my work, TCP ports (even ephemeral ones) are a coveted resource (for long, complex and boring reasons), and random processes listening on tcp ports can cause issues. I know this might be a specific use-case, but worth considering?

It could be a thing to think about. We could do it later though if we decide to go that route. The thing that right now concerns me about it is the windows support.

I don't know exactly if this is an issue or not, but does the working directory of the jedihttp process matter in any meaningful way? I mean currently if you open a python file in Vim and then :cd to another directory then we invoke jedi, does the working directory of jedi change with Vim? It could be meaningful to things like import directories? What I'm wondering is should/could we pass the Vim working directory to JediHTTP on each request, now that we pass it from Vim client.

That is a thing to consider, specially looking forward to the multiple JediHTTP solution for virtualenv and multiple python version support, but I don't know the vim current working directory is really necessary for include directories: wen we ask jedi to generate completions we pass the absolute path of the file, so it know where to look at for relative paths.

I've taken a look at the JediHTTP code, but I couldn't find a simple way to add any line comments. Is there a way to get it into reviewable.io or some equivalent?

I will see if I can do something about it :)

Review status: 11 of 12 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

Comments from the review on Reviewable.io

puremourning · 2015-10-04T15:18:39Z

The thing that right now concerns me about it is the windows support.
FWIW Windows has roughly equivalent support in NT named pipes

Review status: 11 of 12 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

Comments from the review on Reviewable.io

puremourning · 2015-10-04T15:19:04Z

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

Comments from the review on Reviewable.io

vheon · 2015-10-04T15:29:57Z

So with a little abstraction we could do it cross-platform. If that is the case then we could discuss the pro/cons about this and the cost of maintaining it in the future. I think it is better discussing this is ycm-dev or in another issue 👍

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

Comments from the review on Reviewable.io

vheon · 2015-10-04T15:31:04Z

Anyway I made a PR of the master branch against an empty branch of JediHTTP vheon/JediHTTP#1 so you can use reviewable for that as well 👍

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

Comments from the review on Reviewable.io

reinhrst · 2015-10-05T08:42:33Z

Few idle thoughts:

It would be cool if we could use a unix domain socket, rather than a tcp port where that feature is available. Using host ports can cause conflicts, and the mechanism we use typically suffers from a (rare) race condition. I know that waitress supports it because i have a branch which implements it for YCM-ycmd communication. On systems at my work, TCP ports (even ephemeral ones) are a coveted resource (for long, complex and boring reasons), and random processes listening on tcp ports can cause issues. I know this might be a specific use-case, but worth considering?

I think this makes a lot of sense to consider (was on my todo list to check why this path wasn't chosen in the first place). I can also imagine it would make sense to communicate over stdin/stdout instead of starting a server all together. I don't think we have any situation where anyone else would benefit from connecting to a live jedihttp process. Probably better indeed to discuss this ycmd-wide.

I don't know exactly if this is an issue or not, but does the working directory of the jedihttp process matter in any meaningful way? I mean currently if you open a python file in Vim and then :cd to another directory then we invoke jedi, does the working directory of jedi change with Vim? It could be meaningful to things like import directories? What I'm wondering is should/could we pass the Vim working directory to JediHTTP on each request, now that we pass it from Vim client.

[...]I don't know the vim current working directory is really necessary for include directories[...]

It certainly is; I indeed had the feeling that I was missing some completions in vim, and this is exactly the reason why (technically, it's not so much the cwd that is the issue, but the PYTHONPATH, to which . is added automatically) . I made a small example repo to show the issue: go to bar/baz.py, and try to have anything completed from foo. My sys.path when HttpJedi is started (on OSX) looks like this:

'/Users/reinoud/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/JediHTTP/vendor/waitress',
'/Users/reinoud/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/JediHTTP/vendor/jedi',
'/Users/reinoud/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/JediHTTP/vendor/bottle',
'/Users/reinoud/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/JediHTTP/jedihttp', 
'/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python27.zip', 
'/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7', 
'/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-darwin', 
'/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-mac', 
'/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-mac/lib-scriptpackages', 
'/System/Library/Frameworks/Python.framework/Versions/2.7/Extras/lib/python', 
'/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/lib-tk', 
'/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/lib-old', 
'/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/lib-dynload', 
'/System/Library/Frameworks/Python.framework/Versions/2.7/Extras/lib/python/PyObjC', 
'/Library/Python/2.7/site-packages'

As far as I could quickly see, this (plus the directory of the file itself) is searched for a module. This means that in the example the module foo is indeed missed (since only the bar directory is seached). Ideally the /Users/reinoud/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/JediHTTP/* should not be in the search path, and the vim working directory (or something more intelligent....) should.

vheon · 2015-10-05T19:49:50Z

@reinhrst ok so having

'/Users/reinoud/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/JediHTTP/vendor/waitress',
'/Users/reinoud/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/JediHTTP/vendor/jedi',
'/Users/reinoud/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/JediHTTP/vendor/bottle',

in the process path is not ideal because it would bring completion that the user do not actually want/use. How should I use them then? I believe the same is true for /Users/reinoud/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/JediHTTP/jedihttp right?

reinhrst · 2015-10-05T20:02:16Z

Indeed. I guess it should be possible to remove everything you don't want from the sys.path after importing all the modules, but this feels very uncomfortable. Ideally we'd be able to add and remove paths for jedi's search path, but I've been unable to find more about this than this old issue: davidhalter/jedi#36 , and this points towards code that's been removed since. Perhaps we could get @davidhalter to weigh in on what should be the best way to do this now.

davidhalter · 2015-10-06T11:53:08Z

I like this a lot. We have a plan to do a general purpose server for Jedi: davidhalter/jedi#385. So this would be a very good place for us to gather some experience with it.

As for the sys.path modifications, I don't think there's anything that can be done right now. We're working on davidhalter/jedi#562 which will solve modifying the sys path. However, you can do that right now as well, just by replacing the sys.path (it's not very "clean" though).

vheon · 2015-10-06T18:25:53Z

@davidhalter just to be clear: davidhalter/jedi#562 would allow to pass the sys.path jedi should use to do its completion? Do you have an estimate on when that would be ready?

davidhalter · 2015-10-06T23:01:38Z

@vheon Pretty soon. Maybe a month? I'm discussing a split of the issue with the maintainer of the PR. However, I don't see a release coming very soon. But you could just use the latest master.

reinhrst · 2015-10-07T09:42:25Z

Regardless of the way we take forward, we still need to know the current directory (actually the directory of the entrypoint for your python program) to either add to sys.path or to pass to jedi. The extra paths currently passed are a small nuisance that could lead to wrong functionality in very specific cases, however failing to pass the (current dir/entry point dir) leads to a big degradation of the plugin for larger python projects.

I'm just wondering whether anyone knows what's in the sys.path of the ycmd process (which is what jedi currently uses). If the (current dir/entry point dir) is not in there, then we might decide not solving that issue until a later date.

vheon · 2015-10-07T21:14:01Z

@davidhalter

@vheon Pretty soon. Maybe a month? I'm discussing a split of the issue with the maintainer of the PR. However, I don't see a release coming very soon. But you could just use the latest master.

Just to know if it worth it to merge this in and then do the sys.path fix or just wait and do it once 👍 For the release I don't care that much, I would pull in the master branch anyway :) Anyway I understood well the PR?

@reinhrst

Regardless of the way we take forward, we still need to know the current directory (actually the directory of the entrypoint for your python program) to either add to sys.path or to pass to jedi. The extra paths currently passed are a small nuisance that could lead to wrong functionality in very specific cases, however failing to pass the (current dir/entry point dir) leads to a big degradation of the plugin for larger python projects

If I'm not mistaken we already pass the vim current directory with every request, @puremourning did a PR recently, right?

(actually the directory of the entrypoint for your python program)

I don't think that that is going to be an automatic thing. I see that more like a ycm_extra_flags.py file in the root directory or a function to call that should give us the root project directory or something like that.

I think I'll have some time this weekend to look at the "directory-thing"

puremourning · 2015-10-10T12:59:59Z

If I'm not mistaken we already pass the vim current directory with every request, @puremourning did a PR recently, right?

I think we do on completion requests, but not all subcommands, etc. Wouldn't be hard to add, and I think using that is the right way to go.

puremourning · 2016-01-03T18:10:36Z

in_builtin_module is True and is_keyword is False

so, we should be raising an exception with "Builtin modules cannot be displayed." in this case?

  def _BuildGoToResponse( self, definition_list ):
    if len( definition_list ) == 1:
      definition = definition_list[ 0 ]
      if definition[ 'in_builtin_module' ]:
        if definition[ 'is_keyword' ]:
          raise RuntimeError( 'Cannot get the definition of Python keywords.' )
        else:
          raise RuntimeError( 'Builtin modules cannot be displayed.' )
      else:
        return responses.BuildGoToResponse( definition[ 'module_path' ],
                                            definition[ 'line' ],
                                            definition[ 'column' ] + 1 )
    else:
      # multiple definitions
      defs = []
      for definition in definition_list:
        if definition[ 'in_builtin_module' ]:
          defs.append( responses.BuildDescriptionOnlyGoToResponse(
                       'Builtin ' + definition[ 'description' ] ) )
        else:
          defs.append(
            responses.BuildGoToResponse( definition[ 'module_path' ],
                                         definition[ 'line' ],
                                         definition[ 'column' ] + 1,
                                         definition[ 'description' ] ) )
      return defs

I do see the exception text in vanilla YCM, but not this fork (get the empty QF list). So something in the above is not working right.

Review status: all files reviewed at latest revision, 8 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2016-01-03T18:12:44Z

BTW i checked and the Jedi commit on JediHTTP and ycmd are the same so can't be a regression on that side.

vheon · 2016-01-03T19:33:18Z

I'm doing more inspection and there are more than one thing that are not right:

using jedi directly

content = """class Class():
   propertyA = 1
"""
script = jedi.Script( content, 2, 2, '/foo' )
script.goto_definitions()
[]
script = jedi.Script( content, 2, 3, '/foo' )
script.goto_definitions()
[<Definition class int>]

gives two distinct results if the col is 2 or 3, now considering that jedi has column 0-based then it seems that jedi is returning different results depending if we are on the p or the r or propertyA; did I missed something? I wanted to be sure before opening a bug on jedi.

the current implementation of the jedi completer is based on the jedi.NotFoundError which has been deprecated in version v0.9.0 (the one ycmd currently use) and is not thrown anymore; so I'm actually a little puzzled how the current implementation is working 😕
.

Review status: all files reviewed at latest revision, 8 unresolved discussions.

Comments from the review on Reviewable.io

vheon · 2016-01-03T20:00:41Z

So basically with the example above, if I'm am on the p of propertyA then I got the empty QuickFix List while if I am on every other letter of propertyA then I get the exception message. To me looks like a jedi bug.

Review status: all files reviewed at latest revision, 8 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2016-01-03T20:24:45Z

Interesting, and agree that the behaviour of Jedi is dodgy

However, we still have an issue our side, because with this test program:

class A():
  propertyA = 1

And current ycmd, running YcmCompleter GoToDefinition:

on the 'p' of propertyA: "RuntimeError: Can't jump to definition"
on the 'r' of propertyA: "RuntimeError: Builtin modules cannot be displayed"
on the ' ' before propertyA: "RuntimeError: Can't jump to definition"

With this fork:

on the 'p' of propertyA: empty qickfix list
on the 'r' of propertyA: "RuntimeError: Can't jump to definition"
on the ' ' before propertyA: empty qickfix list

So the behaviour is regressed either way (independent of Jedi bug, which is invariant).

Review status: all files reviewed at latest revision, 8 unresolved discussions.

Comments from the review on Reviewable.io

vheon · 2016-01-03T20:35:57Z

Yes, I have to check for empty list of definitions which means that we cannot jump to a definition. I probably have to revisit the JediHTTP API for this and make sure to test this on this end and on JediHTTP itself

Review status: all files reviewed at latest revision, 8 unresolved discussions.

Comments from the review on Reviewable.io

micbou · 2016-01-04T20:39:47Z

I confirm that this is now fixed. However, there is still a little regression in that less informations is given to the user about the error. See the testcase from @puremourning. In the new version, only the RuntimeError: Can't jump to definition will be displayed to the user. A solution is to append the RuntimeErrormessage from _GetDefinitionsList and _BuildGoToResponse to the one in _GoToDefinition, _GoToDeclaration, and _GetDoc (it should not be needed for _GoTo). It would give more informations to the user on why the command failed.

Reviewed 1 of 4 files at r24.
Review status: 16 of 19 files reviewed at latest revision, 11 unresolved discussions.

ycmd/completers/python/jedi_completer.py, line 262 [r24] (raw file):
to instead of do.

ycmd/completers/python/jedi_completer.py, line 290 [r24] (raw file):
Is this error message still appropriate? I cannot obtain it when my cursor is not on a character.

ycmd/completers/python/jedi_completer.py, line 295 [r24] (raw file):
No capital letter for definition.

Comments from the review on Reviewable.io

vheon · 2016-01-04T22:22:26Z

I think is enough to let the more specific exception prevail and we would get the old behaviour :) Thanks!

Review status: 16 of 19 files reviewed at latest revision, 11 unresolved discussions.

Comments from the review on Reviewable.io

vheon · 2016-01-04T22:30:01Z

Review status: 16 of 19 files reviewed at latest revision, 10 unresolved discussions.

ycmd/completers/python/jedi_completer.py, line 290 [r24] (raw file):
It was there on the old completer and I left it there. To explain better: to raise that exception the _GetResponse should throw which means that the response from JediHTTP was a 500. For that to happen it means that you have passed a cursor position that do not exists in the file; in that case the error message would make sense. IIRC is possible to configure vim to be able to have the cursor in such position (I'm not really sure about this) so in that case (after I push my latest changes) you would get that error message. So I would prefer to leave it.

ycmd/completers/python/jedi_completer.py, line 295 [r24] (raw file):
That is going away

Comments from the review on Reviewable.io

micbou · 2016-01-04T23:00:38Z

Review status: 16 of 19 files reviewed at latest revision, 10 unresolved discussions.

ycmd/completers/python/jedi_completer.py, line 253 [r25] (raw file):
else branch is not needed. Maybe it should be the opposite:

if not definitions:
  raise ...
return ...

ycmd/completers/python/jedi_completer.py, line 261 [r25] (raw file):
Same as above.

Comments from the review on Reviewable.io

puremourning · 2016-01-04T23:06:35Z

Reviewed 4 of 4 files at r24, 1 of 1 files at r25.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2016-01-04T23:08:54Z

Review status: 18 of 19 files reviewed at latest revision, 10 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2016-01-04T23:09:14Z

Reviewed 1 of 1 files at r26.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

Comments from the review on Reviewable.io

micbou · 2016-01-05T00:33:02Z

. Waiting for the [READY] tag.

Note: this PR worsens the shutdown issue on Windows. I really need to work on PR #282!

Reviewed 2 of 4 files at r24, 1 of 1 files at r26.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

Comments from the review on Reviewable.io

Valloric · 2016-01-05T03:29:48Z

Awesome work that has earned its badge. :)

Did someone file a bug with Jedi upstream? Let's not lose that, we seem to have a nice test case.

Review status: all files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/handlers_test.py, line 45 [r18] (raw file):
Agreed with @puremourning. Thanks! :)

Not in this PR, but we should come up with a better way of doing this than patching private variables. What we have leads to tight coupling and breaks encapsulation. Someone renames _server_state and boom, tests break.

Comments from the review on Reviewable.io

Valloric · 2016-01-05T03:31:11Z

One last thing, could you rebase and squash the commits into one before we merge? I don't think we need 32 commits of history here.

Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

vheon · 2016-01-05T10:37:01Z

Waiting for the [READY] tag.

Done

Did someone file a bug with Jedi upstream? Let's not lose that, we seem to have a nice test case.

I already did; you can find it at davidhalter/jedi#671. If you read the response one can now understand why jedi has 0-based column, or at least it make some sense now.

One last thing, could you rebase and squash the commits into one before we merge? I don't think we need 32 commits of history here.

Done

Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

micbou · 2016-01-05T15:06:39Z

@homu r+

Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

homu · 2016-01-05T15:06:41Z

📌 Commit c42739a has been approved by micbou

homu · 2016-01-05T15:06:52Z

⚡ Test exempted - status

@reinhrst

[READY] Use a http wrapper for jedi This is the first step to support python completions for python version different from the one ycmd is currently running. # What this change does The jedi library is still used, but instead of calling it directly we now wrapped in a http+JSON server. # Testing ~~The test have been updated to solicit the completer first with a `FileReadyToParse` event, which would start a JediHTTP server if one is not running (similarly to the `OmnisharpServer` tests). The tests for the `GoTo` subcommand family have been updated to test all three of the subcommands.~~ # Notes Two subcommand have been added: `RestartServer` and `StopServer` but only the `RestartServer` is exposed through the user by the `DefinedSubcommand` method, this is because I think that the `StopServer` command is not useful for the user but is useful in the testing phase for shutting down the JediHTTP. I have one question: would make sense to rename any reference from `jedi_completer` to `python_completer`? If it is I will make another PR. @reinhrst If you could test this would be awesome :) Fixes ycm-core/YouCompleteMe#1827  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/246)

lithammer reviewed Sep 28, 2015
View reviewed changes

vheon mentioned this pull request Jan 3, 2016

goto_definitions() return different results if on first char of attribute davidhalter/jedi#671

Closed

vheon force-pushed the feature-jedihttp branch from bbfa285 to c4d07a5 Compare January 4, 2016 14:13

Make python completer use JediHTTP

c42739a

vheon force-pushed the feature-jedihttp branch from 7b6abd2 to c42739a Compare January 5, 2016 10:33

vheon changed the title ~~Use a http wrapper for jedi~~ [READY] Use a http wrapper for jedi Jan 5, 2016

homu merged commit c42739a into ycm-core:master Jan 5, 2016

homu mentioned this pull request Jan 5, 2016

[READY] Add support for the Rust programming language #268

Merged

13 tasks

vheon mentioned this pull request Jan 10, 2016

[READY] Make python binary specifiable by the user #295

Merged

vheon deleted the feature-jedihttp branch February 28, 2016 11:10

[READY] Use a http wrapper for jedi #246

[READY] Use a http wrapper for jedi #246

Conversation

vheon commented Sep 27, 2015

What this change does

Testing

Notes

reinhrst commented Sep 28, 2015

vheon commented Sep 28, 2015

puremourning commented Sep 28, 2015

vheon commented Sep 28, 2015

lithammer Sep 28, 2015

Choose a reason for hiding this comment

lithammer Sep 28, 2015

Choose a reason for hiding this comment

vheon Sep 28, 2015

Choose a reason for hiding this comment

puremourning commented Sep 28, 2015

Valloric commented Sep 29, 2015

vheon commented Sep 30, 2015

micbou commented Oct 1, 2015

Valloric commented Oct 2, 2015

vheon commented Oct 2, 2015

vheon commented Oct 2, 2015

puremourning commented Oct 4, 2015

vheon commented Oct 4, 2015

vheon commented Oct 4, 2015

puremourning commented Oct 4, 2015

puremourning commented Oct 4, 2015

vheon commented Oct 4, 2015

vheon commented Oct 4, 2015

reinhrst commented Oct 5, 2015

vheon commented Oct 5, 2015

reinhrst commented Oct 5, 2015

davidhalter commented Oct 6, 2015

vheon commented Oct 6, 2015

davidhalter commented Oct 6, 2015

reinhrst commented Oct 7, 2015

vheon commented Oct 7, 2015

puremourning commented Oct 10, 2015

puremourning commented Jan 3, 2016

puremourning commented Jan 3, 2016

vheon commented Jan 3, 2016

vheon commented Jan 3, 2016

puremourning commented Jan 3, 2016

vheon commented Jan 3, 2016

micbou commented Jan 4, 2016

vheon commented Jan 4, 2016

vheon commented Jan 4, 2016

micbou commented Jan 4, 2016

puremourning commented Jan 4, 2016

puremourning commented Jan 4, 2016

puremourning commented Jan 4, 2016

micbou commented Jan 5, 2016

Valloric commented Jan 5, 2016

Valloric commented Jan 5, 2016

vheon commented Jan 5, 2016

micbou commented Jan 5, 2016

homu commented Jan 5, 2016

homu commented Jan 5, 2016