Skip to content

Create hls-plugin-api and move plugins to exe#379

Merged
jneira merged 6 commits into
haskell:masterfrom
jneira:hls-plugin-api
Sep 8, 2020
Merged

Create hls-plugin-api and move plugins to exe#379
jneira merged 6 commits into
haskell:masterfrom
jneira:hls-plugin-api

Conversation

@jneira

@jneira jneira commented Sep 6, 2020

Copy link
Copy Markdown
Member

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?

@jneira
jneira requested review from alanz and pepeiborra September 6, 2020 16:49
@jneira
jneira force-pushed the hls-plugin-api branch 2 times, most recently from 2b97604 to f650d75 Compare September 6, 2020 17:03
@pepeiborra

Copy link
Copy Markdown
Collaborator

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

defaultMain :: [PluginDescriptor] -> IO ()

@pepeiborra pepeiborra left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jneira

jneira commented Sep 6, 2020

Copy link
Copy Markdown
Member Author

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

jneira commented Sep 6, 2020

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Collaborator

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 Ailrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jneira

jneira commented Sep 7, 2020

Copy link
Copy Markdown
Member Author

@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

Copy link
Copy Markdown
Collaborator

@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

alanz commented Sep 7, 2020

Copy link
Copy Markdown
Collaborator

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

georgefst commented Sep 7, 2020

Copy link
Copy Markdown
Collaborator

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

jneira commented Sep 7, 2020

Copy link
Copy Markdown
Member Author

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

jneira commented Sep 8, 2020

Copy link
Copy Markdown
Member Author

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

@jneira

jneira commented Sep 8, 2020

Copy link
Copy Markdown
Member Author

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 alanz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jneira
jneira merged commit ec82770 into haskell:master Sep 8, 2020
@jneira
jneira deleted the hls-plugin-api branch September 29, 2020 06:29
pepeiborra pushed a commit that referenced this pull request Dec 27, 2020
* Require shake-0.18.4 which contains actionBracket

* Change progress reporting to use files rather than Shake nodes

* Remove inadvertantly writing down Shake twice
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants