Create hls-plugin-api and move plugins to exe by jneira · Pull Request #379 · haskell/haskell-language-server

jneira · 2020-09-06T16:49:05Z

This is a initial step to acomplish Project architecture to support plugins #164. There @alanz proposed:

I propose the we split hls up

a slimmed-down core, that basically corresponds to the plugins built into ghcide.

an API package that can be used by plugin authors to write plugins for use in hls. The corresponds to the hie-plugin-api as originally envisaged.

a fat package that depends on the core, the api, and all known plugins at this time, and provides an exe depending on them.

The second one would be the hls-plugin-api and the second one the exe component in this pr.

Not sure about:

the place of actual plugins: maybe the folder could be named builtin instead default?
should ghcide be in the api or with the rest of plugins?

pepeiborra · 2020-09-06T17:45:14Z

Maybe add a defaultMain function to hls-plugin-api with the type below, and redefine exe/Main.hs with it?

defaultMain :: [PluginDescriptor] -> IO ()

pepeiborra

This is great! Definitely a step in the right direction. Eventually we can move the current plugins into separate packages too, as needed.

jneira · 2020-09-06T19:31:32Z

Maybe add a defaultMain function to hls-plugin-api with the type below, and redefine exe/Main.hs with it?
defaultMain :: [PluginDescriptor] -> IO ()

Mmm i like to put the main logic in the library like f.e. dhall, if it does not introduce extraneous dependencies.
A drawback could be that, if hls-plugin-api ends up in hackage, you have to make uploads and version bumps for executable related things.

jneira · 2020-09-06T19:42:46Z

ci nix build failed with:

plugins/default/src/Ide/Plugin/Fourmolu.hs:75:11: error:
    Not in scope: `cfgPrinterOpts'
   |
75 |         , cfgPrinterOpts = printerOpts
   |           ^^^^^^^^^^^^^^
cabal: Failed to build exe:haskell-language-server from
haskell-language-server-0.4.0.0 (which is required by test:func-test from
haskell-language-server-0.4.0.0).

🤔

pepeiborra · 2020-09-06T21:42:12Z

ci nix build failed with:

plugins/default/src/Ide/Plugin/Fourmolu.hs:75:11: error:
    Not in scope: `cfgPrinterOpts'
   |
75 |         , cfgPrinterOpts = printerOpts
   |           ^^^^^^^^^^^^^^
cabal: Failed to build exe:haskell-language-server from
haskell-language-server-0.4.0.0 (which is required by test:func-test from
haskell-language-server-0.4.0.0).

🤔

Nix uses cabal build, so that error is probably incorrect version bounds for fourmulu. Not sure why it came up on this change, maybe it's broken in master too?

Ailrun

Great! I had planned to do this but couldn't find an enough time slot...
Thank you for extracting plugin API!

jneira · 2020-09-07T05:38:20Z

@alanz @pepeiborra i dont feel hls-plugin-api is the best place to put the defaultMain, what do you think in keep the package main lib and put it (with Ide.Version and Arguments) there? We have to maintain one more component (although already existing in master) but it would have a better separation of concerns.

pepeiborra · 2020-09-07T07:18:51Z

@alanz @pepeiborra i dont feel hls-plugin-api is the best place to put the defaultMain, what do you think in keep the package main lib and put it (with Ide.Version and Arguments) there? We have to maintain one more component (although already existing in master) but it would have a better separation of concerns.

That sounds fine to me!

alanz · 2020-09-07T07:35:12Z

I agree. The hls-plugin-api should be the thing that tool writers use to create a plugin that is compatible with hls, and has nothing to do with running it.

So in future I would imagine a person writing say a formatter and including a plugin descriptor directly, and the formatter depends on hls-plugin-api.

georgefst · 2020-09-07T18:51:17Z

Nix uses cabal build, so that error is probably incorrect version bounds for fourmulu. Not sure why it came up on this change, maybe it's broken in master too?

The log says it's building fourmolu-0.0.6.0. The ^>= 0.1 bound is in haskell-language-server.cabal for a reason, and this PR appears to have accidentally removed it. This looks to be the case for several other libs as well.

jneira · 2020-09-07T19:32:31Z

The log says it's building fourmolu-0.0.6.0. The ^>= 0.1 bound is in haskell-language-server.cabal for a reason, and this PR appears to have accidentally removed it. This looks to be the case for several other libs as well.

Oops, i forgot to put them when moving the deps around, thanks!

jneira · 2020-09-08T07:00:51Z

Ok, the executable modules are in the lib now and ci is happy (thanks again @georgefst)

jneira · 2020-09-08T08:00:53Z

Rebased, i hope this could be merged asap, cause it will likely be in conflict with any other commit, as it shuffles almost all modules in the project

alanz

LGTM

* Require shake-0.18.4 which contains actionBracket * Change progress reporting to use files rather than Shake nodes * Remove inadvertantly writing down Shake twice

jneira requested review from alanz and pepeiborra September 6, 2020 16:49

jneira force-pushed the hls-plugin-api branch 2 times, most recently from 2b97604 to f650d75 Compare September 6, 2020 17:03

pepeiborra approved these changes Sep 6, 2020

View reviewed changes

Ailrun approved these changes Sep 7, 2020

View reviewed changes

jneira force-pushed the hls-plugin-api branch from 47f1029 to 83571f0 Compare September 8, 2020 07:55

jneira force-pushed the hls-plugin-api branch from 83571f0 to 678b4d5 Compare September 8, 2020 08:31

alanz approved these changes Sep 8, 2020

View reviewed changes

jneira added 6 commits September 8, 2020 22:33

Create hls-plugin-api and move plugins to exe

1c163eb

Keep current version

f3f729e

Add hls-plugin-api component to cradle

489645a

Move exe modules to main library

f9c78d7

Format .cabal files with cabal-fmt --ident 2

d7324a3

Restore ghcide ref

af69f89

jneira force-pushed the hls-plugin-api branch from 678b4d5 to af69f89 Compare September 8, 2020 20:33

jneira merged commit ec82770 into haskell:master Sep 8, 2020

This was referenced Sep 8, 2020

Project architecture to support plugins #164

Closed

Case splitting and lambda introduction #391

Merged

jneira deleted the hls-plugin-api branch September 29, 2020 06:29

pepeiborra pushed a commit that referenced this pull request Dec 27, 2020

Better progress messages (#379)

f3abff8

* Require shake-0.18.4 which contains actionBracket * Change progress reporting to use files rather than Shake nodes * Remove inadvertantly writing down Shake twice

Uh oh!

Conversation

jneira commented Sep 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pepeiborra commented Sep 6, 2020

Uh oh!

pepeiborra left a comment

Choose a reason for hiding this comment

Uh oh!

jneira commented Sep 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jneira commented Sep 6, 2020

Uh oh!

pepeiborra commented Sep 6, 2020

Uh oh!

Ailrun left a comment

Choose a reason for hiding this comment

Uh oh!

jneira commented Sep 7, 2020

Uh oh!

pepeiborra commented Sep 7, 2020

Uh oh!

alanz commented Sep 7, 2020

Uh oh!

georgefst commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jneira commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jneira commented Sep 8, 2020

Uh oh!

jneira commented Sep 8, 2020

Uh oh!

alanz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jneira commented Sep 6, 2020 •

edited

Loading

jneira commented Sep 6, 2020 •

edited

Loading

georgefst commented Sep 7, 2020 •

edited

Loading

jneira commented Sep 7, 2020 •

edited

Loading