Use Fantomas client instead of direct embedding #836

nojaf · 2021-10-08T14:44:21Z

Following up on fsprojects/fantomas#1845, I want to try out the new approach in FSAC/Ionide.
@baronfel I have some specific questions that I will ask in the code, but overall I'd like to hear your thoughts.
Are we on to something here you think?

PS: This worked on my local machine, that alpha-004 is not published yet.

src/FsAutoComplete.Core/Commands.fs

nojaf · 2021-10-08T14:47:34Z

src/FsAutoComplete.Core/Commands.fs

+              let! fantomasResponse = service.FormatDocumentAsync { SourceCode = currentCode; FilePath = (UMX.untag file); IsLastFile = false; Config = None }
+              match fantomasResponse with
+              | { Code = 1; Content = Some code } ->
+                fantomasLogger.info (Log.setMessage (sprintf "Fantomas daemon was able to format \"%A\"" file))


Where are those logs written to?

these logs are written to serilog eventually, which is configured in the Program.fs. the default outputs are to write all logs to stderror, because LSP communication is done over the main stdin/stdout channels.

Any idea how to view these in Rider when attaching to an existing project? Or is it easy to have them written to a rolling file thing?

Sure, you should be able to add a rolling file appender to the logger configuration here, though you will need to add the Serilog.Sinks.File package in order to do so. It would be excellent if you also hooked the configuration of this file up to a CLI option (perhaps --log-file <filepath>?)

src/FsAutoComplete.Core/Commands.fs

nojaf · 2021-10-13T09:57:47Z

Alright, this is getting in a workable state I think.
@baronfel how would you want to tackle this in terms of migration?
We could support both versions at the same time, but I don't think that will really provide us much feedback.
It also adds another layer of temporary complexity, we introduce a setting that would only last a short time and all that.
I'm a bit more inclined to switch and see how it plays out.
The toast showing users that Fantomas needs to be installed could link to a good documentation page.
And potentially we could prompt to install it (locally or globally) from Ionide, although I'm not sure how and if that would work.

Let me know what you think.

src/FsAutoComplete/FsAutoComplete.Lsp.fs

nojaf · 2021-10-13T11:51:31Z

Oh and we probably want to have some basic tests for this I imagine.
Any thoughts there? Installed a global tool first before running the tests?

baronfel · 2021-10-13T14:01:33Z

re: tests I think the current project-based test case model could work here, because you can do local-tool installs to those folders to test specific scenarios. so you'd make a folder representing a project, install fantomas as a local tool, then verify that formatting worked. separately another folder could test the case where we install fantomas for the user, etc, etc.

nojaf · 2021-10-13T14:05:57Z

re:re:tests ah good to hear, then we can indeed have a real test in place.

src/FsAutoComplete/FsAutoComplete.Lsp.fs

src/FsAutoComplete.Core/Commands.fs

nojaf · 2021-10-14T09:03:25Z

@baronfel again, some good stuff today. Man this is a tricky thing to nail down 😋

nojaf · 2021-10-15T07:03:28Z

Hey again @baronfel, getting pretty happy where this sits right now.
A lot of stuff has been simplified on this side.
Could you review the latest state and perhaps list some action points or things that still need addressing?
Many thanks

baronfel · 2021-10-15T19:58:57Z

paket.dependencies

@@ -6,12 +6,13 @@ source https://api.nuget.org/v3/index.json
 # this is the FCS nightly feed, re-enable at your own risk!
 #source https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json
 #source: ./libs
+source C:\Users\fverdonck\Projects\fantomas\bin\


just making a note, we'd want to remove this before final merge

src/FsAutoComplete/FsAutoComplete.Lsp.fs

baronfel · 2021-10-15T20:05:01Z

src/FsAutoComplete/FsAutoComplete.Lsp.fs

@@ -176,15 +176,13 @@ type FSharpLspServer(backgroundServiceEnabled: bool, state: State, lspClient: FS
    let logger = LogProvider.getLoggerByName "LSP"
    let fantomasLogger = LogProvider.getLoggerByName "Fantomas"
    let backgroundService: BackgroundServices.BackgroundService = if backgroundServiceEnabled then BackgroundServices.ActualBackgroundService() :> _ else BackgroundServices.MockBackgroundService() :> _
-    let mutable commands = new Commands(FSharpCompilerServiceChecker(backgroundServiceEnabled, false), state, backgroundService, false)
+    let mutable rootPath : string option = None
+    let mutable commands = new Commands(FSharpCompilerServiceChecker(backgroundServiceEnabled, false), state, backgroundService, false, rootPath)


this is beginning to smell to me. not your code, but just how large Commands is getting.

baronfel · 2021-10-15T20:31:50Z

I'm really happy with this overall, and really excited for the UX of this change, as well as how it insulates the core of FSAC from changes in Fantomas while enabling users to manage fantomas versions independently!

nojaf · 2021-10-16T20:50:39Z

src/FsAutoComplete/FsAutoComplete.Lsp.fs

+                      let dotConfigContent = File.ReadAllText dotConfig
+                      if dotConfigContent.Contains("fantomas-tool") then
+                        // uninstall a older, non-compatible version of fantomas-tool
+                        Process.Start("dotnet", @"tool uninstall fantomas-tool").WaitForExit(5000) |> ignore


Any objection to bringing in https://github.com/Tyrrrz/CliWrap?
As Process.Start can now fail silently.

Nope, go on ahead!

…fantomas-tool.

nojaf · 2021-10-21T06:44:11Z

@baronfel I think the only thing missing here is some sort of unit/integration test.
Otherwise, I believe we are good to go.

nojaf · 2021-10-21T15:03:22Z

The test passed on Windows as well.

baronfel · 2021-10-21T15:18:53Z

Merged! Thank you for this wonderful contribution, I'm very excited to get it out to our users!

nojaf · 2021-10-21T15:19:53Z

You and me both buddy! Many thanks for all the mentoring along the way.

Krzysztof-Cieslak · 2021-10-21T16:02:47Z

🥳 Awesome work @nojaf, thanks a lot!.

dsyme · 2021-11-01T18:59:19Z

src/FsAutoComplete/FsAutoComplete.Lsp.fs

+        | Ok FormatDocumentResponse.UnChanged ->
+          return LspResult.success None
+        | Ok FormatDocumentResponse.ToolNotPresent ->
+              let actions =


Could all this logic be added to Fantomas.Client? Thanks

This is all LSP-specific responses/API usage here, it's not really something that I would expect to drop seamlessly into, say, the VS API. The bit below about prompting especially would be editor-specific, I have zero idea what those interfaces look like.

Prototype usage of Fantomas client.

c4ce8dd

nojaf commented Oct 8, 2021

View reviewed changes

src/FsAutoComplete.Core/Commands.fs Outdated Show resolved Hide resolved

nojaf commented Oct 8, 2021

View reviewed changes

src/FsAutoComplete.Core/Commands.fs Outdated Show resolved Hide resolved

nojaf commented Oct 8, 2021

View reviewed changes

src/FsAutoComplete.Core/Commands.fs Outdated Show resolved Hide resolved

nojaf added 3 commits October 11, 2021 09:01

Extract fantomas service initialization from formatDocument.

5a0e297

Match all format response code.

7e3ab81

Determine isLastFile from project options.

e2ceb23

nojaf commented Oct 12, 2021

View reviewed changes

src/FsAutoComplete.Core/Commands.fs Outdated Show resolved Hide resolved

nojaf commented Oct 12, 2021

View reviewed changes

src/FsAutoComplete.Core/Commands.fs Outdated Show resolved Hide resolved

nojaf added 2 commits October 13, 2021 08:58

Return None when the code was not modified or is ignored.

c1c9712

Add toast if fantomas is not installed.

36f81b9

nojaf commented Oct 13, 2021

View reviewed changes

src/FsAutoComplete/FsAutoComplete.Lsp.fs Outdated Show resolved Hide resolved

Install fantomas when it is not present.

b78f901

nojaf commented Oct 13, 2021

View reviewed changes

src/FsAutoComplete/FsAutoComplete.Lsp.fs Show resolved Hide resolved

baronfel reviewed Oct 13, 2021

View reviewed changes

src/FsAutoComplete/FsAutoComplete.Lsp.fs Outdated Show resolved Hide resolved

Fallback on individual file if no workspace is present.

1df8367

nojaf commented Oct 14, 2021

View reviewed changes

src/FsAutoComplete.Core/Commands.fs Outdated Show resolved Hide resolved

nojaf commented Oct 14, 2021

View reviewed changes

src/FsAutoComplete.Core/Commands.fs Outdated Show resolved Hide resolved

Update to newer implementation of Fantomas.Client.

56110dc

nojaf requested a review from baronfel October 15, 2021 07:03

baronfel reviewed Oct 15, 2021

View reviewed changes

Install fantomas-tool locally if not present.

cf0e89a

nojaf added 2 commits October 16, 2021 22:46

Uninstall older fantomas-tool version first.

9dd6cba

Correct log comment.

f7f392a

nojaf commented Oct 16, 2021

View reviewed changes

nojaf added 4 commits October 16, 2021 22:53

Add message for when file is ignored.

baa8980

Verified .fantomasignore functionality.

1908cf9

Use published version of Fantomas.Client. Install published alpha of …

4b8e00a

…fantomas-tool.

Use CliWrap to execute the .NET cli commands.

1532c2c

add fantomas tool to existing formatting tests

f2f042b

nojaf marked this pull request as ready for review October 21, 2021 14:49

baronfel changed the title ~~Prototype usage of Fantomas client.~~ Use Fantomas client instead of direct embedding Oct 21, 2021

baronfel merged commit f59662d into ionide:master Oct 21, 2021

nojaf deleted the fantomas-daemon branch October 21, 2021 15:19

This was referenced Oct 22, 2021

Migrate to Fantomas.Client fsprojects/fantomas-for-vs#21

Closed

Use Fantomas.Client JetBrains/resharper-fsharp#320

Closed

dsyme reviewed Nov 1, 2021

View reviewed changes

nojaf mentioned this pull request Nov 4, 2021

UX: updating to a newer version of Fantomas #847

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Fantomas client instead of direct embedding #836

Use Fantomas client instead of direct embedding #836

nojaf commented Oct 8, 2021

nojaf Oct 8, 2021

baronfel Oct 8, 2021

nojaf Oct 8, 2021

baronfel Oct 8, 2021

nojaf commented Oct 13, 2021

nojaf commented Oct 13, 2021

baronfel commented Oct 13, 2021

nojaf commented Oct 13, 2021

nojaf commented Oct 14, 2021

nojaf commented Oct 15, 2021

baronfel Oct 15, 2021

baronfel Oct 15, 2021

baronfel commented Oct 15, 2021

nojaf Oct 16, 2021

baronfel Oct 16, 2021

nojaf commented Oct 21, 2021

nojaf commented Oct 21, 2021

baronfel commented Oct 21, 2021

nojaf commented Oct 21, 2021

Krzysztof-Cieslak commented Oct 21, 2021

dsyme Nov 1, 2021

baronfel Nov 1, 2021

Use Fantomas client instead of direct embedding #836

Use Fantomas client instead of direct embedding #836

Conversation

nojaf commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Oct 13, 2021

nojaf commented Oct 13, 2021

baronfel commented Oct 13, 2021

nojaf commented Oct 13, 2021

nojaf commented Oct 14, 2021

nojaf commented Oct 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baronfel commented Oct 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Oct 21, 2021

nojaf commented Oct 21, 2021

baronfel commented Oct 21, 2021

nojaf commented Oct 21, 2021

Krzysztof-Cieslak commented Oct 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment