Various fixes to lsp-fsharp

[drvink]

Changes described in the commit messages and docstrings.

[razzmatazz]

hey @drvink I am thinking about reviewing/testing this, but my question is about your last commit

I was planning about introducing an ability to install fsautocomplete as a global dotnet tool (almost the exact same code as we have it for csharp-ls in lsp-csharp.el). in this PR you apparently are trying to use the locally installed fsautocomplete dotnet tool, but do not provide ability to install it, right?

is there any particular reason you expect fsautocomplete to be installed as a local dotnet tool and not as a global dotnet tool? as I understand the current installation method for fsharp fails for you too, right?

would you mind if I dropped your last commit and:
(a) rewrite of installation of fsautocomplete using dotnet tool install -g fsautocomplete
(b) drop lsp-fsharp-use-dotnet-tool-for-fsac -- as fsac would always be invoked from a dotnet tool package

[razzmatazz]

also, I don't quiet get the reason behind introducing lsp-use-workspace-root-for-server-default-directory var? is this because you're using a "local" version of fsautocomplete dotnet tool and not a global? i.e. in the case of local dotnet too you only have access to the command from workspace root, right?

then I guess this wouldn't be necessary if we require fsautocomplete to be installed as a global tool

[drvink]

Indeed, I didn't add anything for installing it; it would be even simpler than the existing install code, since as of 0.50, FSAC is intended to be installed as a dotnet tool (and only a nupkg is provided in the github releases, no more zip file). I figured I'd get this PR through first, since it doesn't break anything or change any defaults.

I agree the ideal thing would be to try to run it as a global tool by default. However, when I installed fsautocomplete as a global tool, it still didn't work unless I manually made a dotnet-fsautocomplete symlink pointing to the FSAC binary somewhere in my PATH, and then I could run it via dotnet fsautocomplete (you can't run global tools with dotnet tool). I don't know if we should make the symlink as part of installation from lsp-mode--it would probably be rude to do it automatically, because there's no location that's both guaranteed to be in the user's PATH and where it would not be presumptuous to mess with its contents. But we should definitely at least tell the user they have to do it, if not.)

I don't think it hurts to keep lsp-fsharp-use-dotnet-tool-for-fsac and just leave it nil by default, given that people may want it local for any number of reasons.

(I wonder: should we try fallbacks? E.g. try dotnet tool run iff lsp-fsharp-use-dotnet-tool-for-fsac is non-nil; else (or if it fails) try dotnet fsautocomplete (global); else try the old way. I couldn't find an easy way to do it, but it's probably not worth it anyway.)

And yes, you only have access to local dotnet tools if you're in a directory under the workspace root, so I had to add the stuff for lsp-use-workspace-root-for-server-default-directory in order for lsp-mode to be able to run it. I could easily imagine a situation like this coming up again in the future (including with other lsp servers/language "package" systems), so I think it's useful to keep even if your plan is to just install FSAC as a global tool.

[razzmatazz]

ok, my plan is then to:
(a) use your first 2 commits as a base for my PR
(b) rewrite installation/startup mechanism to use global tool version
(c) leave it to you to rebase your last PR so you can provide customization on top of (b)

[razzmatazz]

@drvink I have upped a PR #3350 with your commits (sans the last one)

I suggest we wait for it to be merged and then you can reapply your last commit with non-global fsautocomplete setup on top of master then

[drvink]

@razzmatazz Do you want me to try to rebase what I have on #3350, or should I wait until it's merged?

[razzmatazz]

@razzmatazz Do you want me to try to rebase what I have on #3350, or should I wait until it's merged?

Actually, could you wait a bit -- I got my pc reinstalled twice -- sorry it took so long -- as I still want to add (executable-find "fsautocomplete") check to inform the user he/she has to fix exec-path before merge

I should be able to do it today

you can then rebase on master

[razzmatazz]

I have enabled automerge on #3350, should be merged soon after CIs are complete

[elken]

Might also be worth adding in defcustom-lsp

[yyoncho]

can you resolve conflicts?

[drvink]

@yyoncho Sorry to keep you waiting; rebased and should be fine to merge now.

[yyoncho]

@drvink thank you.

(FTR it is good to ping after force push, GH might not send a mail)