Skip to content

Add support for Clojure/ClojureScript via clojure-lsp #791

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

Merged
merged 1 commit into from
Apr 25, 2019

Conversation

benedekfazekas
Copy link
Contributor

Additionally enable jumping into jarred dependencies if the server is
configured to return the URI in the right format

@codecov
Copy link

codecov bot commented Apr 24, 2019

Codecov Report

Merging #791 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #791   +/-   ##
=======================================
  Coverage   51.51%   51.51%           
=======================================
  Files           5        5           
  Lines          66       66           
=======================================
  Hits           34       34           
  Misses         32       32
Impacted Files Coverage Δ
lsp-clients.el 62.74% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37cce4d...062bc80. Read the comment docs.

Copy link
Member

@yyoncho yyoncho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one minor comment:

lsp-clojure.el Outdated
name))

(lsp-register-client
(make-lsp-client :new-connection (lsp-stdio-connection '("bash" "-c" "clojure-lsp"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you extract the command into defcustom variable for windows users? Even better will be if you can provide default value that will work for windows as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good shout, looking into it

@yyoncho
Copy link
Member

yyoncho commented Apr 25, 2019

I did some testing and it looks promising, there is functionality that is not currently present in cider(e. g. finding references) and the fact that it performs static analysis is nice too. There are several extension methods what we might want to implement (introducing locals, inlining locals, etc). You may

On the bad side it seems like it is not supported very well by company-lsp (cc @tigersoldier). The server requires client-side filtering but company-lsp fails most of the time probably due to incorrect prefix calculation.

@benedekfazekas
Copy link
Contributor Author

yeah indeed. finding references is there in clojure-emacs/refactor-nrepl but only available for clj (not cljs), see details in this thread clojure-emacs/refactor-nrepl#195 clojure-emacs team member, clj-refactor/refactor-nrepl maintainer here btw ;)

also it seems that rewrite-clj based static analyzers are on the rise nowadays also see https://github.com/borkdude/clj-kondo

next step would be to look at integrating the refactorings clojure-lsp offers

Additionally enable jumping into jarred dependencies if the server is
configured to return the URI in the right format.

Command to start server is configurable via a defcustom.
@benedekfazekas
Copy link
Contributor Author

Added the defcustom for the command. It seems that there is an issue tracking the state of windows support https://github.com/snoe/clojure-lsp/issues/25 @snoe, @sparkofreason can you please chip in what would the start server command for windows?

@yyoncho
Copy link
Member

yyoncho commented Apr 25, 2019

Thank you for contributing to lsp-mode!

I will merge it as it is and we could work on exposing the extension methods/windows support and fixing company-lsp.

yeah indeed. finding references is there in clojure-emacs/refactor-nrepl but only available for clj (not cljs), see details in this thread clojure-emacs/refactor-nrepl#195 clojure-emacs team member, clj-refactor/refactor-nrepl maintainer here btw ;)

I am actually clojure/java programmer in real life so thank you for your work on refactor-nrep!. As a side note, is there an issue tracking the effort on making refactor-nrepl work with the latest cider?

PS: By the way, this is not the only possibility for collaboration between emacs-lsp and clojure-emacs. I think that even at this point, dap-mode supports debugging clojurescript and also it won't be hard to be adjusted to work for clojure.

@yyoncho yyoncho merged commit 1e06207 into emacs-lsp:master Apr 25, 2019
@benedekfazekas
Copy link
Contributor Author

thanks for the merge! I am using latest clj-refactor, refactor-nrepl 2.4.0 with latest cider. feel free to file an issue if you have a problem with latest versions or maybe you have a slightly older one?

@yyoncho
Copy link
Member

yyoncho commented Apr 25, 2019

I had some issues in the past and I am checking clj-refactor readme compatibility table from time to time - https://github.com/clojure-emacs/clj-refactor.el . It still says that it supports only cider 0.17/0.18. Is this info out of date?

@benedekfazekas
Copy link
Contributor Author

i suppose it is, my versions

CIDER 0.21.0 (New York)
clj-refactor 2.4.0 (package: 20190117.1235), refactor-nrepl 2.4.0

@yyoncho
Copy link
Member

yyoncho commented Apr 25, 2019

Thank you, I will try it when I am back at work.

@benedekfazekas
Copy link
Contributor Author

let me know if you have any probs, ta!

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.

2 participants