Format out of process #1845

nojaf · 2021-07-29T14:49:57Z

This is an initial attempt at fixing #1844.
Here, a new NuGet package Fantomas.Client is introduced that contains LSPFantomasService (exposed via interface FantomasService).
LSPFantomasService starts a process and offloads the formatting over there using vs-streamjsonrpc.

The FantomasService can be constructed by providing the ProcessStartInfo or using the createForWorkingDirectory helper function. This helper function should be the main choice as it will look for a local or global fantomas-tool.

I still have a couple of open questions:

Should there be a heartbeat mechanism to verify the health of the daemon process?
What to do with logging? I believe the lsp could feed notifications back in from the process to the FantomasService. These could then be exposed via the interface somehow.
What should the response be when Fantomas failed to format a file? (Incomplete AST, a bug in Fantomas). Result type? A more loose error message like a simple string or a more detailed DU type?
The current implementation exposes all methods as F# Async, while under the hood they are Tasks. Does it make sense to expose Task instead?
In the current implementation, the parsingOptions are created by the daemon. The relevance of the F# parsingOptions is to know if a file should have a module or namespace. Scripts and the last file in a project do not require this. How do we want to deal with this?
Related to the previous remark, I'm puzzled how format selection currently even works. I believe the filename would always need to be a script as the selection in the source is passed to parser. Not sure how that is supposed to work for *.fsi files.

baronfel · 2021-09-15T18:34:42Z

My opinions, point by point:

That would be a 'nice to have' IMO, as any critical failures could also be detected by the fantomas daemon shutting down, which would terminate the stdin/stdout pipes
logging could be done either by the $/logTrace or window/logMessage LSP messages, and FSAC could forward those along to the calling LSP client (e.g in VScode) for display
I think the ideal for errors in a client/server system where either side can change without recompilation is error codes that can be linked to for documentation purposes. Internally Fantomas might represent these as a DU, but the external representation could look something very much like { message: string; code: string}?
No opinion here, whatever's easiest for you.
I'm not sure what you mean here specifically, might need more guidance
I'm not sure myself :D

src/Fantomas.Client/Contracts.fs

nojaf · 2021-10-13T11:09:20Z

Things still left to do:

Make sure Fantomas.Client as a manual version v0.1.0 instead of the same version as Fantomas.
Check the restriction on FSharp.Core, this is fixed to 5.0.2 to make it locally work but should not be there or be less strict?
Add a documentation section (or page) that elaborates on why we are doing this and how users should deal with this in their codebase.
Add documentation for the cli tool help page
Add Clear cache method to FantomasService API
Check if the ignore file will work in all scenarios? I'm not totally convinced.
Check if the editor config is being picked up in all scenarios. More confident here.
All filePath parameters should be absolute paths, there should be additional validation for that.
Add unit test for the FantomasDaemon

src/Fantomas.Client/LSPFantomasServiceTypes.fs

* WIP lambda as last argument * Update remaining test according to style proposal. * Add test for multiline non lambda argument. * Print trivia for function keyword from ast range. * multiline non lambda argument, match lambda

baronfel

This all looks wonderful, I am especially happy to see that setting up StreamJsonRpc doesn't appear to have been incredibly hard to get going. Some of the tool-finding and resolution code makes me wonder if there's space for a community library around managing those to abstract that part away, but none of it looks wrong or bad in any way. This is a wonderful set of changes, and one I'm looking very much forward to integrating into FSAC.

baronfel · 2021-10-20T15:47:49Z

src/Fantomas.Client/Fantomas.Client.fsproj

+    <WarningsAsErrors>FS0025</WarningsAsErrors>
+    <PackageLicenseFile>LICENSE.md</PackageLicenseFile>
+    <PackageIcon>fantomas_logo.png</PackageIcon>
+  </PropertyGroup>


would you be open to supporting sourcelink? That would involve:

adding nuget Microsoft.SourceLink.GitHub copy_local: true to paket.dependencies and referencing it in all projects that you wanted to provide sourcelink for

adding <PublishRepositoryUrl>true</PublishRepositoryUrl> to those same projects

adding <EmbedUntrackedSources>true</EmbedUntrackedSources> to those same projects

but then it should just work. if you want to test the outputs of this you can use dotnet-validate via dotnet validate package local <path to nupkg> to make sure the sourcelink support is all correct.

Is this good or do I need to fix these things?

All set now after some changes/conversation in Slack

src/Fantomas.Client/FantomasToolLocator.fs

baronfel

Let's do this!

* Style guide lambda (#1858) * WIP lambda as last argument * Update remaining test according to style proposal. * Add test for multiline non lambda argument. * Print trivia for function keyword from ast range. * multiline non lambda argument, match lambda * WIP Daemon service contract. * Early working daemon prototype. * Take .editorconfig settings into account. * Fix current build. * Add Fantomas locator. * Found globally installed Fantomas version. * Remove unused function. * Add formatSelection. * Refactor FormatDocumentResponse to DU. * Remove unused LspTypes dependency. * Update FCS in Daemon * Refactored public api of FantomasService. * Don't pass full exception in FormatDocumentResponse. * Use separate versioning for Fantomas.Client. * FantomasService manages multiple daemon versions. * Fully implement FantomasService. * Return configuration as json. * Validate if filePath in request is absolute. * Ignore first three 4.6 alphas as compatible tools. * Find ignore file from current file path. * Remove safeFileName from CodeFormatterImpl.fs. Disable daemon tests. * Add daemon unit tests. * Update help text for daemon mode. * Add documentation for Fantomas daemon mode. * Initial attempt at adding source links. * Set DebugType to embedded. * Bump Fantomas.Client to 0.3.0

* Style guide lambda (fsprojects#1858) * WIP lambda as last argument * Update remaining test according to style proposal. * Add test for multiline non lambda argument. * Print trivia for function keyword from ast range. * multiline non lambda argument, match lambda * WIP Daemon service contract. * Early working daemon prototype. * Take .editorconfig settings into account. * Fix current build. * Add Fantomas locator. * Found globally installed Fantomas version. * Remove unused function. * Add formatSelection. * Refactor FormatDocumentResponse to DU. * Remove unused LspTypes dependency. * Update FCS in Daemon * Refactored public api of FantomasService. * Don't pass full exception in FormatDocumentResponse. * Use separate versioning for Fantomas.Client. * FantomasService manages multiple daemon versions. * Fully implement FantomasService. * Return configuration as json. * Validate if filePath in request is absolute. * Ignore first three 4.6 alphas as compatible tools. * Find ignore file from current file path. * Remove safeFileName from CodeFormatterImpl.fs. Disable daemon tests. * Add daemon unit tests. * Update help text for daemon mode. * Add documentation for Fantomas daemon mode. * Initial attempt at adding source links. * Set DebugType to embedded. * Bump Fantomas.Client to 0.3.0

nojaf force-pushed the ras-al-ghul branch 2 times, most recently from a2ba488 to dec961f Compare August 1, 2021 07:44

nojaf force-pushed the 4.6 branch 2 times, most recently from ac0fa7b to 2160491 Compare August 11, 2021 17:26

nojaf force-pushed the 4.6 branch from 8abef9b to 0eda72a Compare August 23, 2021 07:43

nojaf force-pushed the 4.6 branch 2 times, most recently from f20e7f6 to aafa100 Compare September 10, 2021 06:34

nojaf force-pushed the ras-al-ghul branch from cb14c0d to 498dc49 Compare September 10, 2021 06:51

baronfel reviewed Sep 15, 2021

View reviewed changes

src/Fantomas.Client/Contracts.fs Show resolved Hide resolved

nojaf force-pushed the 4.6 branch 2 times, most recently from e414d13 to 1b23ebe Compare September 22, 2021 19:09

nojaf force-pushed the 4.6 branch 2 times, most recently from fed7d42 to 1e8f4d0 Compare September 29, 2021 06:26

nojaf force-pushed the ras-al-ghul branch from 498dc49 to 0a58f5d Compare September 29, 2021 06:36

nojaf force-pushed the 4.6 branch 2 times, most recently from 0156c12 to 4aff4fb Compare October 7, 2021 06:25

nojaf force-pushed the ras-al-ghul branch from 0a58f5d to 9a97c9d Compare October 7, 2021 06:47

nojaf force-pushed the 4.6 branch from 4aff4fb to 50d1af0 Compare October 8, 2021 09:28

nojaf force-pushed the ras-al-ghul branch from 9a97c9d to 2a1b880 Compare October 8, 2021 09:28

nojaf mentioned this pull request Oct 8, 2021

Use Fantomas client instead of direct embedding ionide/FsAutoComplete#836

Merged

nojaf commented Oct 14, 2021

View reviewed changes

src/Fantomas.Client/LSPFantomasServiceTypes.fs Show resolved Hide resolved

nojaf force-pushed the 4.6 branch from 50d1af0 to b7547b9 Compare October 18, 2021 07:14

nojaf force-pushed the ras-al-ghul branch from cb929bb to 977fa0c Compare October 19, 2021 06:58

nojaf force-pushed the 4.6 branch from b7547b9 to 02b127e Compare October 20, 2021 09:01

nojaf added 4 commits October 20, 2021 11:28

Style guide lambda (fsprojects#1858)

9ea1b4c

* WIP lambda as last argument * Update remaining test according to style proposal. * Add test for multiline non lambda argument. * Print trivia for function keyword from ast range. * multiline non lambda argument, match lambda

WIP Daemon service contract.

e28a40f

Early working daemon prototype.

b879731

Take .editorconfig settings into account.

2ca2ae2

Add daemon unit tests.

2a7837f

nojaf force-pushed the ras-al-ghul branch from 977fa0c to 2a7837f Compare October 20, 2021 09:30

nojaf added 2 commits October 20, 2021 11:45

Update help text for daemon mode.

ed04614

Add documentation for Fantomas daemon mode.

85cd808

nojaf marked this pull request as ready for review October 20, 2021 10:09

nojaf requested a review from baronfel October 20, 2021 14:20

baronfel reviewed Oct 20, 2021

View reviewed changes

nojaf added 3 commits October 20, 2021 18:55

Initial attempt at adding source links.

cf31b36

Set DebugType to embedded.

ba2fef9

Bump Fantomas.Client to 0.3.0

298847e

baronfel approved these changes Oct 20, 2021

View reviewed changes

nojaf merged commit db3dc67 into fsprojects:4.6 Oct 20, 2021

nojaf deleted the ras-al-ghul branch October 20, 2021 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format out of process #1845

Format out of process #1845

nojaf commented Jul 29, 2021 •

edited

Loading

baronfel commented Sep 15, 2021

nojaf commented Oct 13, 2021 •

edited

Loading

baronfel left a comment

baronfel Oct 20, 2021

nojaf Oct 20, 2021

baronfel Oct 20, 2021

baronfel left a comment

Format out of process #1845

Format out of process #1845

Conversation

nojaf commented Jul 29, 2021 • edited Loading

baronfel commented Sep 15, 2021

nojaf commented Oct 13, 2021 • edited Loading

baronfel left a comment

Choose a reason for hiding this comment

baronfel Oct 20, 2021

Choose a reason for hiding this comment

nojaf Oct 20, 2021

Choose a reason for hiding this comment

baronfel Oct 20, 2021

Choose a reason for hiding this comment

baronfel left a comment

Choose a reason for hiding this comment

nojaf commented Jul 29, 2021 •

edited

Loading

nojaf commented Oct 13, 2021 •

edited

Loading