[READY] Allow switching to a different JavaScript project with RestartServer #875

micbou · 2017-11-20T21:47:09Z

Contrarily to what the YCM docs say, issuing the :YcmCompleter RestartServer command to switch to a different JavaScript project doesn't work. The reason is that we don't set the working directory when starting the Tern server. Same issue if the user starts the client outside a project and then open a file in that project. It's even worse in that case because no warning will be displayed to the user.

I considered passing the request_data object when initializing the completer but that doesn't work because the request is not always available. I also considered adding support for multiple projects similarly to what the C# completer does but I felt it was too much work. Finally, I went for starting the server on the FileReadyToParse event.

This change is

codecov-io · 2017-11-20T23:39:02Z

Codecov Report

Merging #875 into master will increase coverage by 0.19%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #875      +/-   ##
==========================================
+ Coverage   94.81%   95.01%   +0.19%     
==========================================
  Files          79       79              
  Lines        5440     5455      +15     
  Branches      171      171              
==========================================
+ Hits         5158     5183      +25     
+ Misses        233      225       -8     
+ Partials       49       47       -2

Start Tern on the FileReadyToParse event. Make sure there is only one attempt to start the server. Add Tern configuration path to debugging information.

micbou · 2017-11-24T19:35:27Z

I did some refactoring in the tests by introducing an utility function to start the Tern server. Also, similarly to the recent changes in PR #857, I slightly changed the logic so that we only try to start the Tern server once. PR is ready.

Reviewed 6 of 6 files at r1, 4 of 4 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

bstaletic · 2017-11-25T01:12:22Z

Makes me wonder if we should start all semantic completers on FileReadyToParse.

Reviewed 2 of 6 files at r1, 3 of 4 files at r2, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Valloric · 2017-11-26T17:20:42Z

Thanks!

Review status: all files reviewed at latest revision, 1 unresolved discussion.

ycmd/completers/javascript/tern_completer.py, line 391 at r4 (raw file):

      _logger.info( 'Starting Tern server...' )

      filepath = request_data[ 'filepath' ]

This PR adds 10+ lines of code to a function with a TODO of "this function is too long". :(

Doesn't block this PR, but we should consider refactoring this sooner rather than later.

Comments from Reviewable

bstaletic · 2017-11-27T12:05:08Z

Doesn't block this PR, but we should consider refactoring this sooner rather than later.

So is it time to merge this?

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

Comments from Reviewable

micbou · 2017-11-27T20:54:26Z

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

ycmd/completers/javascript/tern_completer.py, line 391 at r4 (raw file):

Previously, Valloric (Val Markovic) wrote…

This PR adds 10+ lines of code to a function with a TODO of "this function is too long". :(

Doesn't block this PR, but we should consider refactoring this sooner rather than later.

Did some simplifications and moved the new code to a separate function. It's now 54 lines instead of 74.

ycmd/completers/javascript/tern_completer.py, line 442 at r4 (raw file):

      except Exception:
        _logger.exception( 'Unable to start Tern server' )
        self._CleanUp()

Removed this try/except block as it was unlikely to ever catch something (CreateLogFile, OpenForStdHandle, and SafePopen are never supposed to fail).

Comments from Reviewable

puremourning · 2017-11-27T23:00:54Z

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

ycmd/completers/javascript/tern_completer.py, line 157 at r5 (raw file):

    if not FindTernProjectFile( filepath ):
      raise RuntimeError( 'Warning: Unable to detect a .tern-project file '
                          'in the hierarchy before ' + filepath +

should this be a dir name rather than file name? totally optional. i'm ambivalent.

ycmd/completers/javascript/tern_completer.py, line 268 at r5 (raw file):

    with self._server_state_mutex:
      extras = [
        responses.DebugInfoItem( key = 'configuration file',

should we call it "Tern project file", as that is a little clearer that it is specific to Tern rather than YCM ? I theory at least editors and ycmd itself might have "configuration files". Again, optional.

ycmd/completers/javascript/tern_completer.py, line 386 at r5 (raw file):

    if not self._server_project_file:
      _logger.warning( 'No .tern-project file detected: %s', filepath )
      self._server_working_dir = os.path.dirname( filepath )

I think this might be a slight regression, but I'm not sure.

Previously in this scenario we would use Vim's working dir, which might turn out a better choice than the file's directory.

Example might be:

a user is used to using the javascript completion without a tern project file
they typically cd /some/path
the structure is like /some/path/lib_one/*.js, /some/path/lib_two/*.js
they use vim lib_one/file.js, which contains var x = require( 'lib_two/another_file.js' )

I don't know for certain if this would or would not have previously just about worked based on the wd of the term server being /some/path. But I'm pretty sure that it wouldn't with the wd of /some/path/lib_one.

I guess the key question for discussion is: if we can't find a tern project file, should we really use the dirname of the file, or revert to the previous behaviour of using the WD of the client. I suspect that we should do the latter, for safety unless there's a good reason not to... or I totally missed the point, which is of course also possible!

ycmd/completers/javascript/tern_completer.py, line 442 at r5 (raw file):

      if self._ServerIsRunning():
        _logger.info( 'Tern Server started with pid: %s listening on port %s',

should these be %d ? just being pedantic, you can ignore.

ycmd/completers/javascript/tern_completer.py, line 489 at r5 (raw file):

    self._server_started = False
    self._server_working_dir = None
    self._server_project_file = None

should we reset do_tern_project_check here ? I think it's not a new issue, but after restarting the server in some other directory, we probably want to re-scan for the tern project, and also re-warn for the "new" project.

ycmd/tests/javascript/event_notification_test.py, line 160 at r5 (raw file):

  # Finally, restart the server in a folder containing a .tern-project file. We
  # expect no error in that case.

should we test that if we restart it in some other directory (which doesn't have a .tern-project) that we do get another error (as mentioned, I think we don't, but i'm not 100% sure).

Comments from Reviewable

micbou · 2017-11-28T15:34:05Z

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

ycmd/completers/javascript/tern_completer.py, line 157 at r5 (raw file):