Remove JSON transport #1489
Remove JSON transport #1489
Conversation
Also demote Hoogle to Support since it no longer needs to provide commands.
| @@ -22,27 +22,6 @@ import Text.HTML.TagSoup.Tree | |||
|
|
|||
| -- --------------------------------------------------------------------- | |||
|
|
|||
| hoogleDescriptor :: PluginId -> PluginDescriptor | |||
There was a problem hiding this comment.
Hoogle Plugin does not exist anymore? Out of curiosity, what does that mean? Was it previously possible to invoke the PluginCommands "info" and "lookup" without a hover request? Or is this change unobservable for the lsp client?
There was a problem hiding this comment.
Through LSP no, it was for when the JSON transport could run plugin commands directly. The functionality is still there in the lookup/info cmds, just not registered as a command.
| lookupCmd' n term = do | ||
| -- | 'lookup' @n term@ looks up the given Text in the local Hoogle database. | ||
| -- Takes the first @n@ matches. | ||
| -- May fail with a HoogleError that can be shown to the user. |
There was a problem hiding this comment.
Nitpick: Indentation a bit off
|
|
||
| -- --------------------------------------------------------------------- | ||
|
|
||
| baseDescriptor :: PluginId -> PluginDescriptor |
There was a problem hiding this comment.
Same question as for hoogle plugin. What does this entail for lsp-clients?
There was a problem hiding this comment.
This has no change for LSP clients, it was only used for querying information about plugins from the JSON transport and isn't accessible via LSP
| , commandDesc :: T.Text | ||
| , commandFunc :: CommandFunc a b |
There was a problem hiding this comment.
Why is the description removed and the CommandFunc? Are these remnants from JsonStdio? Is the description displayed somewhere for lsp-clients?
There was a problem hiding this comment.
Descriptions and name were only used for the plugin querying commands in base, which aren't accessible through LSP anymore. PluginCommand should now be isomorphic to the LSP commands
There was a problem hiding this comment.
I removed CommandFunc just to simplify things. I think it's more illustrative to have the function type directly visible inside PluginCommand and it removes the redundant
xCmd :: CmdFunc a bxCmd' :: a -> IdeGhcM (IdeResult b)pattern
|
|
||
| newtype CommandFunc a b = CmdSync (a -> IdeGhcM (IdeResult b)) |
There was a problem hiding this comment.
Why is this removed? Dont we need it anymore? Just for understanding the PR.
app/MainHie.hs
Outdated
| , hsimportDescriptor "hsimport" | ||
| , liquidDescriptor "liquid" | ||
| , packageDescriptor "package" | ||
| , pragmasDescriptor "pragmas" | ||
| , floskellDescriptor "floskell" | ||
| , biosDescriptor "bios" | ||
| , genericDescriptor "generic" |
There was a problem hiding this comment.
Why are we removing the base, hoogle and bios plugins?
There was a problem hiding this comment.
The base plugin was only used by the JSON transport to query existing plugins, LSP can't access them
[ PluginCommand "version" "return HIE version" versionCmd
, PluginCommand "plugins" "list available plugins" pluginsCmd
, PluginCommand "commands" "list available commands for a given plugin" commandsCmd
, PluginCommand "commandDetail" "list parameters required for a given command" commandDetailCmd
Same goes for Hoogle and Bios, they only exposed commands that LSP can't access, so it didn't make much sense to keep them around as plugins
| , pluginCommands = | ||
| [ PluginCommand "applyOne" "Apply a single hint" applyOneCmd | ||
| , PluginCommand "applyAll" "Apply all hints to the file" applyAllCmd | ||
| , PluginCommand "lint" "Run hlint on the file to generate hints" lintCmd |
There was a problem hiding this comment.
The lint command isn't accessible via LSP, i.e. Server.hs just manually calls lint to send off the diagnostic notifications, and it doesn't make any sense for this to be a command in LSP (I don't think commands can really return anything or than workspace edits?). applyOne/applyAll are still there since code actions can call them
| -- | Add a package to the project's dependencies. | ||
| -- May fail if no project dependency specification can be found. | ||
| -- Supported are `*.cabal` and `package.yaml` specifications. | ||
| -- Moreover, may fail with an IOException in case of a filesystem problem. |
There was a problem hiding this comment.
This is one of the benefits of removing the CommandFunc wrapper, as it allows us to coalesce all the xCmd xCmd' functions into just one. This should be an NFC
lukel97
force-pushed
the
remove-json-stdio-2
branch
from
7ca1293 to
aa0f0c5
Compare
src/Haskell/Ide/Engine/Version.hs
Outdated
| {-# LANGUAGE PartialTypeSignatures #-} | ||
| {-# LANGUAGE TemplateHaskell #-} | ||
| module Haskell.Ide.Engine.Plugin.Base where | ||
| {-# LANGUAGE CPP, TemplateHaskell, OverloadedStrings #-} |
There was a problem hiding this comment.
Why are you collapsing the pragmas?
There was a problem hiding this comment.
I am not sure. I think this came in when shuffling parts about from the old PR. Will fix
PR haskell#1489 removed the --lsp option, resulting in a hard failure if a client uses it to start up the server. Parse it, but ignore it, as an interim measure.
A fresh take on #1105
Stuff to do