-
Notifications
You must be signed in to change notification settings - Fork 766
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
[READY] JavaScript completer using Tern.js #272
Conversation
Prototype supports: - completion - GetType - GetDoc - GoTo and GoToDefinition - automatic server management
…gging. Write logs to files to work with test suite
…l broken due to tern.js behaviour
This also seems to resolve issues around completions with a query string. \o/ Add more tests for completions and a basic test for subcommands (before rebasing on master where this approach has been changed) Improve some of the test utilities to allow specifying unsaved buffer contents and to check documentation, etc. on completions. Tidy up documentation output for completions and for GetDoc. Mimic tern_for_vim for the former, but use our own style for the latter.
Fix a bug where these would return spurious JSON error when run without an identifier - catch the HTTP error that tern returns. Also add GoToReferences which returns a list of the refs
The "installation" requires running 'npm install --production' in the third-party/tern directory. We detect the lack of the tern_modules directory and disable our completer. This ensures that anyone not specifying '--tern-completer' doesn't get it and therefore it won't conflict with existing users' tern_for_vim installations.
…lter than for them to never appear
HMMMMMM. The appveyor builds are failing, unable to find npm in build.py. I've tried two approaches: @micbou any clues why that might be? I can see that npm must be there because appveyor.yaml has uses it |
I think I got it. it's |
Grumble. Very grumble. The problem is https://bugs.python.org/issue2200 i.e. the extension |
Welcome to the joy of Windows.
You can reuse the I am not reviewing the tests because they will change with PR #270. Review status: 0 of 27 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. ycmd/completers/javascript/tern_completer.py, line 164 [r1] (raw file): ycmd/completers/javascript/tern_completer.py, line 199 [r1] (raw file): ycmd/completers/javascript/tern_completer.py, line 280 [r1] (raw file): Comments from the review on Reviewable.io |
Thanks @micbou 'preciate it! I've fired up my Windows VM to get this to work. :) Review status: 0 of 27 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. Comments from the review on Reviewable.io |
So strange. Now i'm getting strange EPERM errors when tern is started by my completer, but not when run manually. The completer runs it with the following command line:
But the process dies immediately with:
If I run the same command from the command line, it works. If I run it in python interactive shell, it works too.
The only difference seems to be the "shortened" path name with the tilde Clues? Tips? ;_; |
Even with the same path in command line python it works:
I must be going mad. I even tried with raw Popen rather than More grumble :p |
Let me try your Review status: 0 of 27 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. Comments from the review on Reviewable.io |
i pushed the fixed to build.py just now |
thanks, btw. sorry to be a pain :( |
Redirecting stdout and stderr to the logfiles makes the server crash. Need to understand why. Review status: 0 of 27 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. Comments from the review on Reviewable.io |
Interesting. It works if i reduce it all into one file and run it manually: import os
import sys
sys.path.append( os.path.join( os.path.dirname( __file__ ), '..', '..', '..' ) )
print sys.path
from ycmd.utils import ( PathToFirstExistingExecutable, SafePopen,
GetUnusedLocalhostPort, PathToTempDir )
PATH_TO_TERNJS_BINARY = os.path.abspath(
os.path.join(
os.path.dirname( __file__ ),
'..',
'..',
'..',
'third_party',
'tern',
'bin',
'tern' ) )
node_path = PathToFirstExistingExecutable( [ 'node' ] )
if not node_path:
raise RuntimeError( 'Unable to find node executable' )
else:
print ( 'Using node binary from: ' + node_path )
server_port = GetUnusedLocalhostPort()
if True:
extra_args = [ '--verbose' ]
else:
extra_args = []
command = [ node_path,
PATH_TO_TERNJS_BINARY,
'--port',
str( server_port ),
'--host',
'127.0.0.1',
'--persistent',
'--no-port-file' ] + extra_args
print( 'Starting tern with the following command: ' + ' '.join( command ) )
logfile_format = os.path.join( PathToTempDir(), u'tern_{port}_{std}.log' )
server_stdout = logfile_format.format(
port = server_port,
std = 'stdout' )
server_stderr = logfile_format.format(
port = server_port,
std = 'stderr' )
try:
with open( server_stdout, 'w' ) as stdout:
with open( server_stderr, 'w' ) as stderr:
server_handle = SafePopen( command,
stdout = stdout,
stderr = stderr )
except Exception:
print( 'Unable to start Tern.js server' )
server_port = 0
if ( server_port > 0 and server_handle is not None
and server_handle.poll() is None ):
print ( 'Tern.js Server started with pid: ' +
str( server_handle.pid ) +
' listening on port ' +
str( server_port ) )
print ( 'Tern.js Server log files are: ' +
server_stdout +
' and ' +
server_stderr )
server_handle.wait() Then i can use |
Hmmm. Yes. Removing stdout= and stderr= from Popen call fixes it, but the output from the server is lost in the ether. subprocess.PIPE also causes it to crash (and also loses the output). |
I even hacked something within waitress/bottle and that works, so it isn't that either. |
Reviewed 1 of 2 files at r4, 2 of 2 files at r5. build.py, line 30 [r3] (raw file): The duplication is shitty but we'll live with it. Comments from the review on Reviewable.io |
I've marked this as [READY], though AppVeyor is my nemesis today - seemingly impossibly slow. I'll see if I can massage it. Also, do you want me to rebase this into one commit ? Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke. Comments from the review on Reviewable.io |
I think there is an issue with AppVeyor. All tests pass but the run does not finish. It could be that the Tern server is still running. Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke. Comments from the review on Reviewable.io |
that's what i was just thinking. I will check it out. |
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke. ycmd/completers/javascript/tern_completer.py, line 412 [r5] (raw file): Comments from the review on Reviewable.io |
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke. ycmd/completers/javascript/tern_completer.py, line 412 [r5] (raw file): I also have tons of node processes on my windows system, so it is obviously the problem! Comments from the review on Reviewable.io |
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke. ycmd/completers/javascript/tern_completer.py, line 412 [r5] (raw file): Comments from the review on Reviewable.io |
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke. ycmd/completers/javascript/tern_completer.py, line 412 [r5] (raw file): Comments from the review on Reviewable.io |
Since this is likely caused by us not killing the Tern server properly, this is great instance of AppVeyor saving our butt by catching a nasty issue before it went out to users! :) Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke. Comments from the review on Reviewable.io |
There seems to be a wider issue. Basically the @atexit stuff isn't actually firing, so I think this project has increased my respect for the folk who develop for windows (or actually cross-platform stuff) in a more general capacity. It takes a special type of genius to work in this environment 😀 Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke. ycmd/completers/javascript/tern_completer.py, line 412 [r5] (raw file): Comments from the review on Reviewable.io |
status changed back to [WIP] Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke. Comments from the review on Reviewable.io |
Yes, I know about this issue. It was discussed in ycm-core/YouCompleteMe#876 but no satisfactory solution was found. This is on my list of things to improve on Windows. Anyway, I see that you decided to manually stop the server and this is the right thing to do. Even without this issue, you don't want to pile up all those Tern servers and kill them in one go at the end. Review status: 22 of 28 files reviewed at latest revision, 1 unresolved discussion. Comments from the review on Reviewable.io |
I see, so we have 2 known issues on windows:
The former is not (yet) well-understood and the latter is a "known" issue on windows. It also looks like the AppVeyor tests are now passing (woo!). So based on that, switching this back to [READY] Review status: 22 of 28 files reviewed at latest revision, 1 unresolved discussion. Comments from the review on Reviewable.io |
I'm fine with merging this with the tern server still running after vim exit on Windows because we have the same issue with OmniSharp, but that's a really nasty problem. It was sorta fine before because we didn't officially support Windows, but now that we do, it's just looks bad. With that, @homu r+ Review status: 22 of 28 files reviewed at latest revision, all discussions resolved. Comments from the review on Reviewable.io |
📌 Commit 1a24c89 has been approved by |
[READY] JavaScript completer using Tern.js # Overview Adds support for a JavaScript completer using [Tern.js](http://ternjs.net). The completer supports: * Intelligent auto-completion * Go to definition, references (`GoTo`, etc.) * Semantic type information for identifiers (`GetType`) * View documentation comments for identifiers (`GetDoc`) * Management of `tern` server instance This provides (for Vim at least) a replacement, for [tern_for_vim](https://github.com/ternjs/tern_for_vim) automagically in ycmd. ## Demo Completions: ![ycm-tern-demo](https://cloud.githubusercontent.com/assets/10584846/11763113/035f748c-a0f6-11e5-8961-77139d4e6f45.gif) Subcommands: ![ycm-tern-demo-subcommands](https://cloud.githubusercontent.com/assets/10584846/11763115/0523467c-a0f6-11e5-8552-357bcdda2e6a.gif) ## Installation Run `install.py --tern-completer` ## Caveats and differences vs. tern_for_vim * Currently, to use it there must be a `.tern-project` configuration file somewhere in the directory tree above Vim's working directory when the first JavaScript file is opened (i.e. when the server starts). I believe this is also true for `tern_for_vim`. * Does not support refactoring (rename) which `tern_for_vim` does * Methods from `Object.prototype` are always included. Tern has the ability to only include them when they match some number of typed characters, as they are otherwise "noise". This is incompatible with ycmd's cache and LCS matching. ## Status I believe this to be fully working with the above caveats. I think the first one is the most frustrating, but it isn't obvious how to avoid it. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/272) <!-- Reviewable:end -->
☀️ Test successful - status |
Should we write it somewhere? |
Overview
Adds support for a JavaScript completer using Tern.js.
The completer supports:
GoTo
, etc.)GetType
)GetDoc
)tern
server instanceThis provides (for Vim at least) a replacement, for tern_for_vim automagically in ycmd.
Demo
Completions:
Subcommands:
Installation
Run
install.py --tern-completer
Caveats and differences vs. tern_for_vim
.tern-project
configuration file somewhere in the directory tree above Vim's working directory when the first JavaScript file is opened (i.e. when the server starts). I believe this is also true fortern_for_vim
.tern_for_vim
doesObject.prototype
are always included. Tern has the ability to only include them when they match some number of typed characters, as they are otherwise "noise". This is incompatible with ycmd's cache and LCS matching.Status
I believe this to be fully working with the above caveats. I think the first one is the most frustrating, but it isn't obvious how to avoid it.