[READY] Overhaul Go completer #505

micbou · 2016-05-22T16:24:11Z

This PR includes the changes mentioned in PR #282 for the Go completer and other small changes (listed in the commit message). The main change is the way we start the gocode daemon: instead of letting the client automatically start the daemon, we directly start it by running the gocode executable in server mode (using the -s parameter). This allows us to:

get its PID;
log stdout and stderr into files;
enable gocode debug mode;
set the port listened by gocode;
terminate the process;
implement the _ServerIs* methods;
etc.

The second important change is the implementation of the DebugInfo method to show debug informations about the Go completer. The way informations are displayed will be adapted to other completers. Here's the output obtained in Vim when editing a Go file before and after the changes:

Before:

Printing YouCompleteMe debug information...
-- Server has Clang support compiled in: True
-- Clang version: clang version 3.8.0 (branches/release_38)
--
-- Server running at: http://127.0.0.1:59642
-- Server process ID: 4884
-- Server logfiles:
--   c:\users\micbou\appdata\local\temp\ycm_temp\server_59642_stdout.log
--   c:\users\micbou\appdata\local\temp\ycm_temp\server_59642_stderr.log

How can I write a good issue report in these conditions?

After:

Printing YouCompleteMe debug information...
-- Server has Clang support compiled in: True
-- Clang version: clang version 3.8.0 (branches/release_38)
-- Go completer debug information:
--   Gocode running at: http://127.0.0.1:59667
--   Gocode process ID: 8720
--   Gocode binary: C:\Users\micbou\VIM~1\bundle\YOUCOM~2\THIRD_~1\ycmd\third_pa
rty\gocode\gocode.exe
--   Gocode logfiles:
--     C:\Users\micbou\AppData\Local\Temp\ycm_temp\gocode_59667_stdout.log
--     C:\Users\micbou\AppData\Local\Temp\ycm_temp\gocode_59667_stderr.log
--   Godef binary: C:\Users\micbou\VIM~1\bundle\YOUCOM~2\THIRD_~1\ycmd\third_par
ty\godef\godef.exe
-- Server running at: http://127.0.0.1:59663
-- Server process ID: 2788
-- Server logfiles:
--   c:\users\micbou\appdata\local\temp\ycm_temp\server_59663_stdout.log
--   c:\users\micbou\appdata\local\temp\ycm_temp\server_59663_stderr.log

Please help, I am overwhelmed by all this information.

Finally, the last important change (copied from the Python completer) is replacing the StartServer subcommand with the RestartServer one (reasoning is that if you can restart the server, you can start it too) and hiding the StopServer subcommand from the user (if users need to stop the server then something is wrong with the completer).

This change is

Valloric · 2016-05-22T16:34:52Z

Excellent stuff! Thanks for taking this on.

Some comments left.

Previously, micbou wrote…

[READY] Overhaul Go completer

This PR includes the changes mentioned in PR #282 for the Go completer and other small changes (listed in the commit message). The main change is the way we start the gocode daemon: instead of letting the client automatically start the daemon, we directly start it by running the gocode executable in server mode (using the -s parameter). This allows us to:

get its PID;

log stdout and stderr into files;

enable gocode debug mode;

set the port listened by gocode;

terminate the process;

implement the _ServerIs* methods;

etc.

The second important change is the implementation of the DebugInfo method to show debug informations about the Go completer. The way informations are displayed will be adapted to other completers. Here's the output obtained in Vim when editing a Go file before and after the changes:

Before:
Printing YouCompleteMe debug information...

-- Server has Clang support compiled in: True

-- Clang version: clang version 3.8.0 (branches/release_38)

--

-- Server running at: http://127.0.0.1:59642

-- Server process ID: 4884

-- Server logfiles:

--   c:\users\micbou\appdata\local\temp\ycm_temp\server_59642_stdout.log

--   c:\users\micbou\appdata\local\temp\ycm_temp\server_59642_stderr.log
How can I write a good issue report in these conditions?

After:
Printing YouCompleteMe debug information...

-- Server has Clang support compiled in: True

-- Clang version: clang version 3.8.0 (branches/release_38)

-- Go completer debug information:

--   Gocode running at: http://127.0.0.1:59667

--   Gocode process ID: 8720

--   Gocode binary: C:\Users\micbou\VIM~1\bundle\YOUCOM~2\THIRD_~1\ycmd\third_pa

rty\gocode\gocode.exe

--   Gocode logfiles:

--     C:\Users\micbou\AppData\Local\Temp\ycm_temp\gocode_59667_stdout.log

--     C:\Users\micbou\AppData\Local\Temp\ycm_temp\gocode_59667_stderr.log

--   Godef binary: C:\Users\micbou\VIM~1\bundle\YOUCOM~2\THIRD_~1\ycmd\third_par

ty\godef\godef.exe

-- Server running at: http://127.0.0.1:59663

-- Server process ID: 2788

-- Server logfiles:

--   c:\users\micbou\appdata\local\temp\ycm_temp\server_59663_stdout.log

--   c:\users\micbou\appdata\local\temp\ycm_temp\server_59663_stderr.log
Please help, I am overwhelmed by all this information.

Finally, the last important change (copied from the Python completer) is replacing the StartServer subcommand with the RestartServer one (reasoning is that if you can restart the server, you can start it too) and hiding the StopServer subcommand from the user (if users need to stop the server then something is wrong with the completer).

Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions.

ycmd/completers/go/go_completer.py, line 127 [r1] (raw file):

  def _ComputeOffset( self, contents, line, col ):

This does'n seem like it needs to be a member func; it can just be a private func in the module.

The benefit if not making it a member func is that it's obvious it doesn't mutate any GoCompleter object state, improving readability & maintainability.

ycmd/completers/go/go_completer.py, line 130 [r1] (raw file):

    """Compute the byte offset in the file given the line and column."""
    contents = ToBytes( contents )
    curline = 1

I'd expand the name to current_line. Similar for line below.

ycmd/completers/go/go_completer.py, line 142 [r1] (raw file):

    _logger.error( 'Go completer could not compute byte offset '
                   'corresponding to L%i C%i', line, col )
    return -1

Is returning -1 ok? No need for an exception to abort the request?

ycmd/completers/go/go_completer.py, line 145 [r1] (raw file):

  def _ConvertCompletionData( self, completion_data ):

Similar for _ComputeOffset; doesn't seem like it needs to be a member func.

ycmd/completers/go/go_completer.py, line 316 [r1] (raw file):

                                       '-o={0}'.format( offset ) ],
                                     contents = contents )
    except RuntimeError as error:

Can we catch a more specific exception type if possible?

ycmd/completers/go/go_completer.py, line 351 [r1] (raw file):

                              'status' ] )
      return True
    except RuntimeError as error:

More specific exception?

ycmd/completers/go/go_completer.py, line 370 [r1] (raw file):

               '    {3}\n'
               '    {4}\n'
               '  Godef binary: {5}'.format( self._gocode_address,

Probably needs to be wrapped in the lock since it access the data protected by it. Both reads & writes need the lock.

Comments from Reviewable

coveralls · 2016-05-22T16:45:02Z

Coverage decreased (-0.08%) to 84.878% when pulling daed0ff on micbou:go-completer-overhaul into 994b7eb on Valloric:master.

micbou · 2016-05-22T17:07:48Z

Reviewed 5 of 5 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

ycmd/completers/go/go_completer.py, line 127 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

This does'n seem like it needs to be a member func; it can just be a private func in the module.

The benefit if not making it a member func is that it's obvious it doesn't mutate any GoCompleter object state, improving readability & maintainability.

I reverted the change.

ycmd/completers/go/go_completer.py, line 130 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

I'd expand the name to current_line. Similar for line below.

Done. For `curcol` too.

ycmd/completers/go/go_completer.py, line 142 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

Is returning -1 ok? No need for an exception to abort the request?

I replaced `offset` to `-1` to check `gocode` behavior and it panicked so, as you suggested, raising an exception here is better than catching the one from `gocode`.

ycmd/completers/go/go_completer.py, line 145 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

Similar for _ComputeOffset; doesn't seem like it needs to be a member func.

Done.

ycmd/completers/go/go_completer.py, line 316 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

Can we catch a more specific exception type if possible?

No because we want to catch the `RuntimeError` exception that may be raised in the `_ExecuteCommand` method. Also, we are logging the exception so we should have all the information we need to debug the exception.

ycmd/completers/go/go_completer.py, line 351 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

More specific exception?

See my comment above.

ycmd/completers/go/go_completer.py, line 370 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

Probably needs to be wrapped in the lock since it access the data protected by it. Both reads & writes need the lock.

We'll need to do the same for other completers using a lock.

Comments from Reviewable

coveralls · 2016-05-22T17:25:45Z

Coverage decreased (-0.1%) to 84.836% when pulling f401a26 on micbou:go-completer-overhaul into 994b7eb on Valloric:master.

coveralls · 2016-05-22T17:25:51Z

Coverage decreased (-0.08%) to 84.878% when pulling f401a26 on micbou:go-completer-overhaul into 994b7eb on Valloric:master.

Valloric · 2016-05-23T22:31:58Z

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

ycmd/completers/go/go_completer.py, line 316 [r1] (raw file):

Previously, micbou wrote…

No because we want to catch the RuntimeError exception that may be raised in the _ExecuteCommand method. Also, we are logging the exception so we should have all the information we need to debug the exception.

Ok, but leave a comment about it.

ycmd/completers/go/go_completer.py, line 351 [r1] (raw file):

Previously, micbou wrote…

See my comment above.

Comment needed.

ycmd/completers/go/go_completer.py, line 370 [r1] (raw file):

Previously, micbou wrote…

We'll need to do the same for other completers using a lock.

Agreed.

Comments from Reviewable

Valloric · 2016-05-23T22:32:11Z

Previously, coveralls wrote…

Coverage decreased (-0.08%) to 84.878% when pulling f401a26 on micbou:go-completer-overhaul into 994b7eb on Valloric:master.

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

Comments from Reviewable

micbou · 2016-05-24T16:50:11Z

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

ycmd/completers/go/go_completer.py, line 316 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

Ok, but leave a comment about it.

Done.

ycmd/completers/go/go_completer.py, line 351 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

Comment needed.

Done.

Comments from Reviewable

coveralls · 2016-05-24T17:38:19Z

Coverage decreased (-0.1%) to 84.836% when pulling f36f014 on micbou:go-completer-overhaul into 994b7eb on Valloric:master.

Valloric · 2016-05-24T19:06:12Z

I see coverage decreasing. Not good. Could we add some more tests? Idea is to aim for >=95% test coverage for new code.

coveralls · 2016-05-25T03:27:22Z

Coverage increased (+0.3%) to 85.246% when pulling 5ff026c on micbou:go-completer-overhaul into 22f229c on Valloric:master.

micbou · 2016-05-25T12:06:29Z

I added tests for the DebugInfo method, a test for _ComputeOffset when the offset cannot be computed, and a test for when GoTo cannot jump to definition.

Previously, coveralls wrote…

Coverage increased (+0.3%) to 85.246% when pulling 5ff026c on micbou:go-completer-overhaul into 22f229c on Valloric:master.

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

Comments from Reviewable

micbou · 2016-05-25T21:58:42Z

Move the DebugInfo tests to a separate file. They are unrelated to subcommands.

Previously, micbou wrote…

I added tests for the DebugInfo method, a test for _ComputeOffset when the offset cannot be computed, and a test for when GoTo cannot jump to definition.

Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

coveralls · 2016-05-25T22:24:41Z

Coverage increased (+0.3%) to 85.246% when pulling dd3463b on micbou:go-completer-overhaul into 22f229c on Valloric:master.

vheon · 2016-05-25T22:39:13Z

Previously, coveralls wrote…

Coverage increased (+0.3%) to 85.246% when pulling dd3463b on micbou:go-completer-overhaul into 22f229c on Valloric:master.

Reviewed 3 of 5 files at r1, 1 of 1 files at r3, 1 of 3 files at r4, 1 of 2 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

ycmd/completers/go/go_completer.py, line 316 [r1] (raw file):

Previously, micbou wrote…

Done.

I don't think I understood the comment 😕 can we reword it a little? :(

Comments from Reviewable

micbou · 2016-05-25T23:27:28Z

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

ycmd/completers/go/go_completer.py, line 316 [r1] (raw file):

Previously, vheon (Andrea Cedraro) wrote…

I don't think I understood the comment 😕 can we reword it a little? :(

I reworded it. Hope it is more understandable this way.

Comments from Reviewable

coveralls · 2016-05-25T23:44:44Z

Coverage increased (+0.3%) to 85.246% when pulling 9bb2745 on micbou:go-completer-overhaul into 22f229c on Valloric:master.

coveralls · 2016-05-26T00:09:13Z

Coverage increased (+0.3%) to 85.246% when pulling 9bb2745 on micbou:go-completer-overhaul into 22f229c on Valloric:master.

vheon · 2016-05-26T19:52:10Z

Previously, coveralls wrote…

Coverage increased (+0.3%) to 85.246% when pulling 9bb2745 on micbou:go-completer-overhaul into 22f229c on Valloric:master.

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

Comments from Reviewable

Improve Go completer with the following changes: - start GoCode as a server; - add locks when starting and stopping the server; - create logfiles for GoCode; - implement DebugInfo method; - add StopServer and RestartServer subcommands; - do not expose the StopServer command to users; - improve exception handling for GoTo subcommand; - implement ServerIsRunning, ServerIsHealthy, and ServerIsReady methods; - change error message when no completions found; - update Gocode to latest commit; - update Go binaries paths; - rewrite Go tests; - add tests for DebugInfo, DefinedSubcommands, and _ComputeOffset methods; - add test for when unable to jump to definition; - raise an exception if unable to compute offset.

coveralls · 2016-06-04T09:15:08Z

Coverage increased (+0.3%) to 85.266% when pulling 3fe7ea5 on micbou:go-completer-overhaul into 47304e1 on Valloric:master.

Valloric · 2016-06-06T22:15:16Z

@micbou Any specific reason why you're not merging this? You have two LGTMs.

micbou · 2016-06-06T23:03:18Z

No particular reason. Let's merge it.

@homu r+

Previously, Valloric (Val Markovic) wrote…

@micbou Any specific reason why you're not merging this? You have two LGTMs.

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

Comments from Reviewable

homu · 2016-06-06T23:03:21Z

📌 Commit 3fe7ea5 has been approved by micbou

homu · 2016-06-06T23:03:33Z

⌛ Testing commit 3fe7ea5 with merge 24ccb3b...

[READY] Overhaul Go completer This PR includes the changes mentioned in PR #282 for the Go completer and other small changes (listed in the commit message). The main change is the way we start the `gocode` daemon: instead of letting the client automatically start the daemon, we directly start it by running the `gocode` executable in server mode (using the `-s` parameter). This allows us to: - get its PID; - log stdout and stderr into files; - enable `gocode` debug mode; - set the port listened by `gocode`; - terminate the process; - implement the `_ServerIs*` methods; - etc. The second important change is the implementation of the `DebugInfo` method to show debug informations about the Go completer. The way informations are displayed will be adapted to other completers. Here's the output obtained in Vim when editing a Go file before and after the changes: Before: ``` Printing YouCompleteMe debug information... -- Server has Clang support compiled in: True -- Clang version: clang version 3.8.0 (branches/release_38) -- -- Server running at: http://127.0.0.1:59642 -- Server process ID: 4884 -- Server logfiles: -- c:\users\micbou\appdata\local\temp\ycm_temp\server_59642_stdout.log -- c:\users\micbou\appdata\local\temp\ycm_temp\server_59642_stderr.log ``` *How can I write a good issue report in these conditions?* After: ``` Printing YouCompleteMe debug information... -- Server has Clang support compiled in: True -- Clang version: clang version 3.8.0 (branches/release_38) -- Go completer debug information: -- Gocode running at: http://127.0.0.1:59667 -- Gocode process ID: 8720 -- Gocode binary: C:\Users\micbou\VIM~1\bundle\YOUCOM~2\THIRD_~1\ycmd\third_pa rty\gocode\gocode.exe -- Gocode logfiles: -- C:\Users\micbou\AppData\Local\Temp\ycm_temp\gocode_59667_stdout.log -- C:\Users\micbou\AppData\Local\Temp\ycm_temp\gocode_59667_stderr.log -- Godef binary: C:\Users\micbou\VIM~1\bundle\YOUCOM~2\THIRD_~1\ycmd\third_par ty\godef\godef.exe -- Server running at: http://127.0.0.1:59663 -- Server process ID: 2788 -- Server logfiles: -- c:\users\micbou\appdata\local\temp\ycm_temp\server_59663_stdout.log -- c:\users\micbou\appdata\local\temp\ycm_temp\server_59663_stderr.log ``` *Please help, I am overwhelmed by all this information.* Finally, the last important change (copied from the Python completer) is replacing the `StartServer` subcommand with the `RestartServer` one (reasoning is that if you can restart the server, you can start it too) and hiding the `StopServer` subcommand from the user (if users need to stop the server then something is wrong with the completer).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/505)

homu · 2016-06-06T23:32:17Z

☀️ Test successful - status

micbou force-pushed the go-completer-overhaul branch from 171f8e6 to daed0ff Compare May 22, 2016 16:30

micbou force-pushed the go-completer-overhaul branch from daed0ff to f401a26 Compare May 22, 2016 16:54

micbou mentioned this pull request May 23, 2016

[READY] Make completer interface uniform throughout completers #506

Merged

micbou force-pushed the go-completer-overhaul branch from f401a26 to f36f014 Compare May 24, 2016 16:49

micbou force-pushed the go-completer-overhaul branch from f36f014 to 5ff026c Compare May 25, 2016 03:05

micbou force-pushed the go-completer-overhaul branch from 5ff026c to dd3463b Compare May 25, 2016 21:57

micbou force-pushed the go-completer-overhaul branch 2 times, most recently from 2a8b7b6 to 9bb2745 Compare May 25, 2016 23:23

micbou force-pushed the go-completer-overhaul branch from 9bb2745 to 3fe7ea5 Compare June 4, 2016 08:45

homu merged commit 3fe7ea5 into ycm-core:master Jun 6, 2016

micbou mentioned this pull request Jun 7, 2016

RuntimeError: Gocode returned empty JSON response. #444

Closed

micbou deleted the go-completer-overhaul branch June 7, 2016 02:29

micbou mentioned this pull request Jul 14, 2016

[READY] Properly shut down on Windows #282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY] Overhaul Go completer #505

[READY] Overhaul Go completer #505

micbou commented May 22, 2016 •

edited by Valloric

Loading

Valloric commented May 22, 2016

[READY] Overhaul Go completer

coveralls commented May 22, 2016

micbou commented May 22, 2016

coveralls commented May 22, 2016

coveralls commented May 22, 2016

Valloric commented May 23, 2016

Valloric commented May 23, 2016

micbou commented May 24, 2016

coveralls commented May 24, 2016

Valloric commented May 24, 2016

coveralls commented May 25, 2016

micbou commented May 25, 2016

micbou commented May 25, 2016

coveralls commented May 25, 2016

vheon commented May 25, 2016

micbou commented May 25, 2016

coveralls commented May 25, 2016

coveralls commented May 26, 2016

vheon commented May 26, 2016

coveralls commented Jun 4, 2016

Valloric commented Jun 6, 2016

micbou commented Jun 6, 2016

homu commented Jun 6, 2016

homu commented Jun 6, 2016

homu commented Jun 6, 2016

[READY] Overhaul Go completer #505

[READY] Overhaul Go completer #505

Conversation

micbou commented May 22, 2016 • edited by Valloric Loading

Valloric commented May 22, 2016

[READY] Overhaul Go completer

coveralls commented May 22, 2016

micbou commented May 22, 2016

coveralls commented May 22, 2016

coveralls commented May 22, 2016

Valloric commented May 23, 2016

Valloric commented May 23, 2016

micbou commented May 24, 2016

coveralls commented May 24, 2016

Valloric commented May 24, 2016

coveralls commented May 25, 2016

micbou commented May 25, 2016

micbou commented May 25, 2016

coveralls commented May 25, 2016

vheon commented May 25, 2016

micbou commented May 25, 2016

coveralls commented May 25, 2016

coveralls commented May 26, 2016

vheon commented May 26, 2016

coveralls commented Jun 4, 2016

Valloric commented Jun 6, 2016

micbou commented Jun 6, 2016

homu commented Jun 6, 2016

homu commented Jun 6, 2016

homu commented Jun 6, 2016

micbou commented May 22, 2016 •

edited by Valloric

Loading