Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Sep 8, 2020

Conversation

jneira
Copy link
Member

@jneira jneira commented Sep 6, 2020

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 force-pushed the hls-plugin-api branch 2 times, most recently from 2b97604 to f650d75 Compare September 6, 2020 17:03
@pepeiborra
Copy link
Collaborator

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

defaultMain :: [PluginDescriptor] -> IO ()

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

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
Copy link
Member Author

jneira commented Sep 6, 2020

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
Copy link
Member Author

jneira commented Sep 6, 2020

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
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?

Copy link
Member

@Ailrun Ailrun left a comment

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
Copy link
Member Author

jneira commented Sep 7, 2020

@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
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
Copy link
Collaborator

alanz commented Sep 7, 2020

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
Copy link
Collaborator

georgefst commented Sep 7, 2020

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
Copy link
Member Author

jneira commented Sep 7, 2020

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
Copy link
Member Author

jneira commented Sep 8, 2020

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

@jneira
Copy link
Member Author

jneira commented Sep 8, 2020

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

Copy link
Collaborator

@alanz alanz left a comment

Choose a reason for hiding this comment

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

LGTM

@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