Skip to content

Implement auto-installation of fsharp language server #920

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 7 commits into from
Jul 5, 2019

Conversation

TOTBWF
Copy link
Collaborator

@TOTBWF TOTBWF commented Jul 5, 2019

This PR implements auto-installation of the F# language server.

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, few comments for improvement:

Not related to this PR but you may consider adding dap-mode debugger support (AFAICS .net/mono debuggers do support debugging F#).

lsp-fsharp.el Outdated

(defun lsp-fsharp--fsac-install ()
"Downloads the latest version of fsautocomplete, and set `lsp-fsharp-server-path'."
(let ((temp-file (make-temp-file "fsautocomplete" nil ".zip")))
Copy link
Member

Choose a reason for hiding this comment

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

Take a look at emacs-lsp/lsp-python-ms#37 which also handles unzipping but it fallbacks to powershell on windows. We probably need to push up that logic in lsp-mode and then reuse it in lsp-python-ms and dap-mode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, fixed.

@@ -77,6 +110,7 @@
:major-modes '(fsharp-mode)
:notification-handlers (ht ("fsharp/notifyCancel" #'ignore)
Copy link
Member

Choose a reason for hiding this comment

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

not part of this review but you may take a look how do we handle the notifications in lsp-rust and lsp-java and implement the same logic here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I'm missing something really obvious, but they both look rather similar to what I'm doing here. Could you point me to the code that you are referencing?

Copy link
Member

Choose a reason for hiding this comment

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

Here it is: https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-rust.el#L296 (although I am not 100% sure that the FSharp notifications are progress notifications).

@@ -121,44 +121,44 @@
~lsp-clients.el~ while others require installing additional packages which provide
server specific functionality.

| Language | Language Server | Built-in | Installation command | Debugger |
Copy link
Member

Choose a reason for hiding this comment

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

The formatting changes are caused by the fact that one is performing the formatting with links display enabled and the other is performing the alignment with link display disabled(use org-toggle-link-display). Can you do the formatting so we will have only the required changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, nice catch. Fixed!

@TOTBWF
Copy link
Collaborator Author

TOTBWF commented Jul 5, 2019

I've been hacking on dap-mode support on and off, but it's a bit of a tricky situation. The debugger that ships with the C# extension for VSCode has a weird license, which only allows for usage inside of Microsoft products. There is netcoredbg, but I've been experiencing some odd behavior with it.

@yyoncho yyoncho merged commit af0c2c6 into emacs-lsp:master Jul 5, 2019
@TOTBWF TOTBWF mentioned this pull request Jul 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.

2 participants