Skip to content
This repository was archived by the owner on Oct 7, 2020. It is now read-only.

Refactor plugins and commands now that JSON transport is gone #1492

Merged
merged 10 commits into from
Dec 24, 2019

Conversation

lukel97
Copy link
Collaborator

@lukel97 lukel97 commented Dec 22, 2019

  • Remove the name and description fields, since they were only used by the JSON transport when querying and can't be seen by LSP
  • Similarly remove the bios plugin as it can't be called by LSP, so there's no point in it being there
  • Remove CommandFunc new type so we can coalesce the cmd/cmd' pattern into just one
xCmd :: CmdFunc a b
xCmd' :: a -> IdeGhcM (IdeResult b)

Verified

This commit was signed with the committer’s verified signature.
ychin Yee Cheng Chin

Verified

This commit was signed with the committer’s verified signature.
ychin Yee Cheng Chin
Now redundant as they were just used to return information via the JSON
transport when querying plugins
No longer need to supply checkCmd (only JSON transport could previously
use it)
Also only needed by the JSON transport
We definitely do not want to get those mixed up with plain old texts
@lukel97 lukel97 requested review from alanz, wz1000 and fendor December 22, 2019 14:11
@lukel97 lukel97 mentioned this pull request Dec 22, 2019
1 task
@lukel97 lukel97 changed the base branch from remove-json-stdio-2 to master December 22, 2019 16:30
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.

Generally in favour, some minor concerns

@lukel97 lukel97 requested review from alanz and fendor December 23, 2019 01:59
@lukel97 lukel97 force-pushed the plugin-command-refactor branch from 8c1b247 to 6a7c076 Compare December 23, 2019 03:20
@mpickering
Copy link
Collaborator

I am strongly against removing pluginName and pluginDescription fields, it seems like a step in the wrong direction.

@alanz
Copy link
Collaborator

alanz commented Dec 23, 2019

I am strongly against removing pluginName and pluginDescription fields, it seems like a step in the wrong direction.

I agree with @mpickering. They are not needed in the current use-case, but the intention has always been to have some kind of discoverability around plugins, and the ability to use alternative ones, e.g. choose the one that supports the formatter you prefer. Having names and descriptions makes this easier.

@lukel97
Copy link
Collaborator Author

lukel97 commented Dec 23, 2019 via email

@alanz
Copy link
Collaborator

alanz commented Dec 23, 2019

@bubba Thanks. That capability does not exist at present, but I think keeping the info in at the moment is worth doing, we can strip it later if we choose to.

Somewhere along the line I would like to see a much more loosely coupled plugin architecture, which allows us to configure per installation, or per project what plugins should be used.

Having meta information available helps with this. And it is likely it may move to a different place, but lets keep the flame alive in the meantime.

Which directly opposes YAGNI, but anyway.

@lukel97 lukel97 merged commit 2c51b61 into haskell:master Dec 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants