Add an option to run Fourmolu via the CLI interface of a separate binary, rather than the bundled library #2763

georgefst · 2022-03-07T23:51:49Z

This makes it easier to set up an environment (e.g. a Nix shell) which uses the same version of Fourmolu in HLS as from the command line.

It can also be useful when Fourmolu has updated and we don't want to wait a month for an HLS release, or re-compile. Or when one wants to try out a development version of Fourmolu.

To do

Add tests
- This will require a non-trivial refactor to parameterise runSessionWithServerFormatterCLI and goldenWithHaskellDocFormatterCLI over a Config arg, so I'd like some confirmation that I'm on the right track first.

Future work

Add config option to editor plugins e.g. VSCode
- ~~We need to note that the option currently only affects Fourmolu~~
- We should make clear that the binary needs to be on PATH
- What should we call this option?
Implement similar functionality for other formatters?
- I'm personally happy to handle Ormolu, but I'm not very familiar with the others
- @tfausak would seemingly be interested in this being implemented for Brittany: Support GHC 9.0 lspitzner/brittany#356 (comment)
We could add an option to specify the exact path to a binary, rather than requiring it to be in PATH. I'm not sure this would be useful though since we already have the haskell.serverEnvironment option. It would also make it harder to have a single option for all formatters. EDIT: is haskell.serverEnvironment specific to VSCode?

Minor issues

lspPrinterOpts is unused. I doubt anyone actually cares. We can't just pass --indentation as this would take priority over options from config files, which is not what we want (see Fix fourmolu plugin inconsistent formatting #599).

…ary, rather than the bundled library This makes it easier to set up an environment (e.g. a Nix shell) which uses the same version of Fourmolu in HLS as from the command line. It can also be useful when Fourmolu has updated and we don't want to wait a month for an HLS release, or re-compile.

michaelpj · 2022-03-08T20:51:19Z

I wonder whether we really want this as a generic option. An alternative would be to just provide an additional formatting provider, say, fourmolu-cli which would call out to the CLI. That way we also don't end up with illegal states when people set that option but pick a formatting provider which doesn't support it.

e.g. what happens if you are trying out formatters and you do:

Set provider to fourmolu
Hmm, maybe I should use the CLI, set that option
Hmm, maybe I'll try stylish-haskell (which doesn't support it)
It breaks?

This will require a non-trivial refactor to parameterise runSessionWithServerFormatterCLI and goldenWithHaskellDocFormatterCLI over a Config arg, so I'd like some confirmation that I'm on the right track first.

I think my suggestion would also solve that: there would simply be some tests for a different formatting provider.

lspPrinterOpts is unused. I doubt anyone actually cares.

Maybe it doesn't matter that much in this case, but I imagine this sort of thing matters more for e.g. passing the right GHC extensions to tools that need to know that.

That's one advantage of my other proposal to do haskell-language-server format foo: you can be sure to use actually the same options as would be provided by HLS.

We could add an option to specify the exact path to a binary, rather than requiring it to be in PATH. I'm not sure this would be useful though since we already have the haskell.serverEnvironment option.

Not all clients are VSCode. Putting it on the PATH is not onerous, but if having an option to give the path is cheap, I think it's nice. Again, no problem since I'm suggesting not having a single option ;)

georgefst · 2022-03-08T23:12:23Z

Thanks @michaelpj. Agreed on all points. I'll implement it as a separate plugin.

lspPrinterOpts is unused. I doubt anyone actually cares.

Maybe it doesn't matter that much in this case, but I imagine this sort of thing matters more for e.g. passing the right GHC extensions to tools that need to know that.

It's fine. GHC options etc. come via fileOpts. lspPrinterOpts only deals with tab size set by the editor itself, outside of HLS.

I may have to think about factoring out convertDynFlags somewhere (where?), rather than duplicating it in a third place (it originally came from the Ormolu plugin.

georgefst · 2022-03-08T23:19:52Z

Actually, I think we'd get most of the benefits, along with easier maintenance, by just making it a plugin-specific option. I'd completely missed that we now have #691.

georgefst · 2022-03-08T23:42:33Z

Putting it on the PATH is not onerous, but if having an option to give the path is cheap, I think it's nice.

It's not immediately obvious to me how to achieve this, and I'm keen to get this PR in to the impending release if possible. So I'll save it for future work.

georgefst · 2022-03-09T00:46:44Z

Right. I think I'm using Properties correctly, but I can't seem to affect any change via editor settings. i.e. adding "haskell.plugin.fourmolu.cli": true in VSCode has no effect. Is there another step needed, or is that just not the right string?

Ensures expected config files are found.

georgefst · 2022-03-09T01:19:11Z

I also can't find a nice way to set this new plugin-specific option in the tests. I could refactor Fourmolu.hs to expose a Bool argument to descriptor instead (or something along those lines), but it's non-trivial...

EDIT: Actually it is pretty straightforward. It's just kind of ugly, and doesn't scale nicely to adding more plugin-specific options.

georgefst · 2022-03-09T01:42:31Z

Somehow 3d9f964 has broken the functionality completely. Odd.

michaelpj · 2022-03-09T10:08:27Z

Actually, I think we'd get most of the benefits, along with easier maintenance, by just making it a plugin-specific option.

Yes, I think you're right. The only issue would be that you might end up with illegal combinations of plugin options? e.g. if you have some other fourmolu option that can't actually take effect when you're using the CLI version. Just a hypothetical worry, though.

adding "haskell.plugin.fourmolu.cli": true in VSCode has no effect.

I think the VSCode extension also needs to be extended so it knows about the new setting, while the config is "generic" on the HLS side, I don't think is on the VSCode side. I don't really know how it works, though.

I'd probably just test it in a grimy way by setting it to true by default 😆

michaelpj · 2022-03-09T10:08:57Z

It's not immediately obvious to me how to achieve this, and I'm keen to get this PR in to the impending release if possible. So I'll save it for future work.

Isn't it just a matter of adding another plugin-specific option? "Path to fourmolu binary (only used if cli option is set)"

georgefst · 2022-03-09T10:19:53Z

It's not immediately obvious to me how to achieve this, and I'm keen to get this PR in to the impending release if possible. So I'll save it for future work.

Isn't it just a matter of adding another plugin-specific option? "Path to fourmolu binary (only used if cli option is set)"

Yes, I'm sure you're right. I started writing that before I'd implemented the first option.

The only issue would be that you might end up with illegal combinations of plugin options?

I don't anticipate that being an issue any time soon.

I'd probably just test it in a grimy way by setting it to true by default 😆

Yeah, don't worry, I've already done that, and it works nicely. Well, it did before the CWD change, which I haven't been able to investigate yet.

I should be able to get this finished up later today.

michaelpj

Implementation looks fine.

michaelpj · 2022-03-10T16:30:45Z

plugins/hls-fourmolu-plugin/src/Ide/Plugin/Fourmolu.hs

+                                <> map ("-o" <>) fileOpts
+                        ){cwd = Just $ takeDirectory fp'}
+                        contents
+                T.hPutStrLn stderr err


this should get logged properly

It's no worse than with the non-CLI version. I'll open a new issue to track better logging for both.

michaelpj · 2022-03-10T16:32:57Z

hls-test-utils/src/Test/Hls.hs

@@ -152,11 +154,14 @@ goldenWithHaskellDocFormatter plugin formatter title testDataDir path desc ext a
 runSessionWithServer :: PluginDescriptor IdeState -> FilePath -> Session a -> IO a
 runSessionWithServer plugin = runSessionWithServer' [plugin] def def fullCaps

-runSessionWithServerFormatter :: PluginDescriptor IdeState -> String -> FilePath -> Session a -> IO a
-runSessionWithServerFormatter plugin formatter =
+runSessionWithServerFormatter :: PluginDescriptor IdeState -> String -> PluginConfig -> FilePath -> Session a -> IO a


Maybe document this function while you're here

plugins/hls-fourmolu-plugin/src/Ide/Plugin/Fourmolu.hs

michaelpj · 2022-03-10T16:33:42Z

plugins/hls-fourmolu-plugin/src/Ide/Plugin/Fourmolu.hs

-                            (printerOpts <> lspPrinterOpts)
-                            defaultPrinterOpts
-                    }
+properties :: Properties '[ 'PropertyKey "cli" 'TBoolean]


bikeshedding: "external"? "shell"? I think "cli" seems fine though.

Yeah, surpisingly no word I could think of here seems perfect.

I've decided I do prefer "external": 558a3bc. Not going to bother changing the variable names though.

Co-authored-by: Michael Peyton Jones <[email protected]>

georgefst · 2022-03-13T16:55:34Z

As for:

Add config option to editor plugins e.g. VSCode

Is there anything I need to do? I see from #1576 that we have some auto-generation script. Is this part of the release process? Is any of this (i.e. the process of adding an option) documented anywhere?

michaelpj · 2022-03-14T11:02:36Z

I have no idea. I suspect it isn't documented: I had a look at the CONTRIBUTING for vscode-haskell and it didn't say. Maybe you can write it down when you find out. @berberman might know?

burkettd · 2022-05-02T15:35:26Z

This looks like this PR adds support for using a locally installed binary of fourmolu instead of the version bundled with hls.

I'm using hls 1.7.0.0, and installed fourmolu 0.6.0.0 on my path. I tried setting "haskell.plugin.fourmolu.cli": true in my coc config for neovim, but it doesn't seem to work because it doesn't seem to apply my import-export-comma-style setting which I think was added in 0.6.0.0.

What settings do I need to set to tell HLS to use a locally installed version of fourmolu?

georgefst · 2022-06-07T16:14:32Z

What settings do I need to set to tell HLS to use a locally installed version of fourmolu?

I genuinely don't know. My personal use case for this disappeared shortly after it was merged. I can only suggest subscribing to #2827.

cydparser · 2023-03-03T02:18:59Z

The setting was later renamed to "external":

{
  "haskell.formattingProvider": "fourmolu",
  "haskell.plugin.fourmolu.config.external": true
}

tablitza · 2024-01-16T14:53:57Z

Using GHC 9.4.8 HLS 2.5.0.0 with lsp-mode on emacs, the bundled fourmolu does not apply indentation from the yaml file, and using the external binary triggers the error fourmolu: Internal Error: fourmolu: createProcess: posix_spawnp: does not exist (No such file or directory)

michaelpj · 2024-01-16T16:56:33Z

Is fourmolu on your editor's path?

tablitza · 2024-01-16T19:31:58Z

Yes, ~/.cabal/bin is on my emacs exec-path variable.

georgefst added 4 commits March 7, 2022 23:35

Minor refactor

9392fb4

Reformat

0c21b0d

Use Text directly for processes

b214278

georgefst mentioned this pull request Mar 7, 2022

Coherence of command-line formatters versus formatter plugins #411

Open

Refactor: Avoid unnecessary monad constraint in convertDynFlags

42f8df3

Make Fourmolu CLI option specific to the Fourmolu plugin

1bd90af

georgefst force-pushed the fourmolu-cli branch from c501fb9 to 1bd90af Compare March 9, 2022 00:47

Set working directory for Fourmolu process

3d9f964

Ensures expected config files are found.

georgefst added 3 commits March 10, 2022 00:39

Fix: Set CWD to containing directory, rather than file

74cab28

Refactor: Allow passing plugin config to goldenWithHaskellDocFormatter

594777a

Test Fourmolu with CLI option

dc64fc4

georgefst marked this pull request as ready for review March 10, 2022 01:28

georgefst requested review from Ailrun, fendor and jneira as code owners March 10, 2022 01:28

michaelpj approved these changes Mar 10, 2022

View reviewed changes

georgefst and others added 3 commits March 10, 2022 16:40

Minor change to flag text

236d244

Co-authored-by: Michael Peyton Jones <[email protected]>

Merge branch 'master' into fourmolu-cli

129e584

Change flag name from "cli" to "external"

558a3bc

georgefst force-pushed the fourmolu-cli branch from fb3975e to 558a3bc Compare March 11, 2022 10:35

georgefst added 2 commits March 11, 2022 11:45

Merge branch 'master' into fourmolu-cli

bb0f45f

Merge branch 'master' into fourmolu-cli

510e180

georgefst merged commit fd34887 into master Mar 11, 2022

This was referenced Mar 22, 2022

Release 1.7.0 #2739

Closed

Remove IDs from definitions, and unify local and global variables hackworthltd/primer#323

Merged

This was referenced Apr 12, 2022

Editor support for Fourmolu CLI option #2827

Closed

Allow specifying Ormolu CLI flags in config file fourmolu/fourmolu#58

Open

georgefst mentioned this pull request May 31, 2022

Use ormolu style for default fourmolu/fourmolu#128

Open

georgefst mentioned this pull request Jul 11, 2022

Bump Fourmolu to 0.7 hackworthltd/primer#563

Merged

sir4ur0n mentioned this pull request Aug 22, 2023

Add support for external Ormolu #3771

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option to run Fourmolu via the CLI interface of a separate binary, rather than the bundled library #2763

Add an option to run Fourmolu via the CLI interface of a separate binary, rather than the bundled library #2763

georgefst commented Mar 7, 2022 •

edited

Loading

michaelpj commented Mar 8, 2022

georgefst commented Mar 8, 2022

georgefst commented Mar 8, 2022

georgefst commented Mar 8, 2022

georgefst commented Mar 9, 2022

georgefst commented Mar 9, 2022 •

edited

Loading

georgefst commented Mar 9, 2022

michaelpj commented Mar 9, 2022

michaelpj commented Mar 9, 2022

georgefst commented Mar 9, 2022

michaelpj left a comment

michaelpj Mar 10, 2022

georgefst Mar 10, 2022 •

edited

Loading

michaelpj Mar 10, 2022

michaelpj Mar 10, 2022

georgefst Mar 10, 2022 •

edited

Loading

georgefst Mar 11, 2022 •

edited

Loading

georgefst commented Mar 13, 2022

michaelpj commented Mar 14, 2022

burkettd commented May 2, 2022

georgefst commented Jun 7, 2022

cydparser commented Mar 3, 2023 •

edited

Loading

tablitza commented Jan 16, 2024

michaelpj commented Jan 16, 2024

tablitza commented Jan 16, 2024

Add an option to run Fourmolu via the CLI interface of a separate binary, rather than the bundled library #2763

Add an option to run Fourmolu via the CLI interface of a separate binary, rather than the bundled library #2763

Conversation

georgefst commented Mar 7, 2022 • edited Loading

To do

Future work

Minor issues

michaelpj commented Mar 8, 2022

georgefst commented Mar 8, 2022

georgefst commented Mar 8, 2022

georgefst commented Mar 8, 2022

georgefst commented Mar 9, 2022

georgefst commented Mar 9, 2022 • edited Loading

georgefst commented Mar 9, 2022

michaelpj commented Mar 9, 2022

michaelpj commented Mar 9, 2022

georgefst commented Mar 9, 2022

michaelpj left a comment

Choose a reason for hiding this comment

michaelpj Mar 10, 2022

Choose a reason for hiding this comment

georgefst Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

michaelpj Mar 10, 2022

Choose a reason for hiding this comment

michaelpj Mar 10, 2022

Choose a reason for hiding this comment

georgefst Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

georgefst Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

georgefst commented Mar 13, 2022

michaelpj commented Mar 14, 2022

burkettd commented May 2, 2022

georgefst commented Jun 7, 2022

cydparser commented Mar 3, 2023 • edited Loading

tablitza commented Jan 16, 2024

michaelpj commented Jan 16, 2024

tablitza commented Jan 16, 2024

georgefst commented Mar 7, 2022 •

edited

Loading

georgefst commented Mar 9, 2022 •

edited

Loading

georgefst Mar 10, 2022 •

edited

Loading

georgefst Mar 10, 2022 •

edited

Loading

georgefst Mar 11, 2022 •

edited

Loading

cydparser commented Mar 3, 2023 •

edited

Loading