Skip to content
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

WIP Fix 1996 - Add eclipse LSP support. #2121

Merged

Conversation

hsanson
Copy link
Contributor

@hsanson hsanson commented Dec 5, 2018

Adds support for the eclipse language server that seems to work better than javalsp for gradle and maven projects.

Closes #1996

ale_linters/java/eclipselsp.vim Outdated Show resolved Hide resolved
@w0rp
Copy link
Member

w0rp commented Dec 5, 2018

Should be good once this is ready to go.

@w0rp
Copy link
Member

w0rp commented Dec 6, 2018

I opened an issue for supporting command_chain for LSP linters. See #2128.

@hsanson hsanson force-pushed the 1996-add-support-for-eclipse-jdt-ls branch from 0958147 to f34e663 Compare January 18, 2019 00:41
@w0rp
Copy link
Member

w0rp commented Jan 27, 2019

This PR is blocked by #2132.

@hsanson hsanson force-pushed the 1996-add-support-for-eclipse-jdt-ls branch from f34e663 to 176d289 Compare January 28, 2019 00:58
@hsanson hsanson force-pushed the 1996-add-support-for-eclipse-jdt-ls branch 2 times, most recently from 5876a14 to 93f5134 Compare April 1, 2019 08:28
@w0rp
Copy link
Member

w0rp commented Apr 8, 2019

@hsanson I finished work on #2132 last night, so now you can replace the system() call here with ale#command#Run. Have a look at the documentation for the function and existing uses in the codebase. You can effectively run the command in the background and receive the results in a callback, which can return the command to run for starting the language server.

@w0rp
Copy link
Member

w0rp commented Apr 8, 2019

You might also want to try using the internal ale#semver#RunWithVersionCheck function, which uses ale#command#Run to parse semver versions from results and then cache the results so we only need to run the version check roughly once per executable.

@hsanson hsanson force-pushed the 1996-add-support-for-eclipse-jdt-ls branch 2 times, most recently from c293971 to be484e0 Compare April 9, 2019 03:27
@hsanson
Copy link
Contributor Author

hsanson commented Apr 9, 2019

@w0rp first let me say, great work. I did not think this refactoring could be done so fast.

Have modified the Eclipse LSP to use the new async callback methods following as example the flake8 linter. Also with caching of versions similar to ale#semver#RunWithVersionCheck.

But I have one test failing it travis CI that I am unable to pinpoint. I am sure I have listed the new LSP support everywhere it should (ale-supported-languages-and-tools.txt, ale.txt, and supported-tools.md).

Also I don't see the LSP being invoked in ALEInfo as before. I can see version checking is being executed but no LSP.

And since a while back I am unable to get auto-completion working with any LSP, including this one. When time allows it, please check this MR and let me know if you find what I am doing wrong.

@w0rp
Copy link
Member

w0rp commented Apr 9, 2019

The test is failing because something isn't sorted correctly in the documentation.

I don't know why the command isn't being run on your machine. I'll have to try this linter myself on my own machine, at some point.

@w0rp w0rp added this to In Review in Old Working List via automation Apr 9, 2019
@hsanson
Copy link
Contributor Author

hsanson commented Apr 10, 2019

Fixed the sorting issue with the list of supported tools.

Using the ch_logfile trick in vim it was easy to find the issue with the LSP server:

  9.856030 on 0: Freeing job
  9.856536 RECV on 1: 'Error: Unable to access jarfile /home/ryujin/Apps/eclipse.jdt.ls/org.eclipse.jdt.ls.product/target/repository/plugins/org.eclipse.equinox.launcher_1.5.200.v20180922-1751.jar
'

In ALEInfo there are no details of the LSP errors. Without the ch_logfile in vim is not possible to know if there are any errors. In neovim this is what I see:

let g:ale_warn_about_trailing_blank_lines = 1
let g:ale_warn_about_trailing_whitespace = 1
  Command History:
(executable check - success) java
(finished - exit code 0) ['/bin/bash', '-c', '''java'' -version']
<<<NO OUTPUT RETURNED>>>

Is there a way I can make any errors reported by LSP appear as part of ALEInfo output?

Apart from the above I think this LSP can be used.

@hsanson hsanson changed the title Fix 1996 - Add eclipse LSP support. WIP Fix 1996 - Add eclipse LSP support. Apr 10, 2019
@hsanson hsanson force-pushed the 1996-add-support-for-eclipse-jdt-ls branch 2 times, most recently from bc54392 to 0119b3f Compare April 10, 2019 14:01
@w0rp
Copy link
Member

w0rp commented Apr 10, 2019

See #2137 for adding LSP debugging information.

@hsanson hsanson force-pushed the 1996-add-support-for-eclipse-jdt-ls branch from 0119b3f to f02e2ec Compare April 10, 2019 23:50
@w0rp w0rp merged commit f0f0cc3 into dense-analysis:master Apr 13, 2019
Old Working List automation moved this from In Review to Done Apr 13, 2019
@w0rp
Copy link
Member

w0rp commented Apr 13, 2019

Cheers! 🍻 Thank you for working on this, waiting for a few months for me to get the uniform asynchronous API done, and then coming back and finishing this.

@JulioJu
Copy link
Contributor

JulioJu commented Apr 15, 2019

@hsanson maybe in the ALE documentation, it could be cool to explain shortly how to customize compiler warning / error preferences ? We should add customization under root_project/.settings/org.eclipse.jdt.core.prefs with values presented at https://help.eclipse.org/neon/topic/org.eclipse.jdt.doc.isv/reference/api/org/eclipse/jdt/core/JavaCore.html ?

What do you think about that ?

@hsanson
Copy link
Contributor Author

hsanson commented Apr 16, 2019

@JulioJu sounds interesting but I have not used this feature and cannot see it documented anywhere on the eclipe lsp repo. Is this a known feature? I do not want to document something that is internal and may change in the future.

@JulioJu
Copy link
Contributor

JulioJu commented Apr 16, 2019

@hsanson you could check eclipse-jdtls/eclipse.jdt.ls#581 . See also for the future redhat-developer/vscode-java#507

@hsanson
Copy link
Contributor Author

hsanson commented Apr 17, 2019

From the issues linked above I see:

We do not have a proper way of defining those yet. unofficially though defining them on an eclipse style .settings/org.eclipse.jdt.core.prefs should work for now.

Once eclipse.jdt.ls has official way to add preferences that is documented I will be more than happy to add a note with a link to the official documentation explaining how to set them.

Of course anyone is free to modify the documentation and submit a MR for review.

JulioJu pushed a commit to JulioJu/ale that referenced this pull request Apr 17, 2019
JulioJu pushed a commit to JulioJu/ale that referenced this pull request Apr 17, 2019
JulioJu pushed a commit to JulioJu/ale that referenced this pull request Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Support eclipse.jdt.ls
4 participants