PluginDescriptor no longer serialisable.

[alanz]

Elisp tests currently fail

Addresses #92

@cocreature or @gracjan please have a look at the elisp tests, the wire format has now changed.

This may also make the ExtendedCommandDescriptor unnecessary

[cocreature]

The format returned by plugins doesn’t make sense. Here’s a simplified response

{
  "plugins": [
    [
      "base",
      [
        {
          "name": "version"
        },
        {
          "name": "plugins"
        }
      ]
    ],
    [
      "eg2",
      [
        {
          "name": "sayHello"
        }
      ]
    ]
  ]
}

The problem is that you are using an array of arrays where each inner array represents a plugin. The inner array always contains only two elements with the first one being the plugin name and the second being an array of plugins.

I don’t see any good reason to not just use a hashmap that maps from plugins to lists of commands. Otherwise the IDE side then needs to convert this array of two element arrays into a map to do anything reasonable with it.

[alanz]

Leaving aside the serialisation, which is completely unoptimised, the key type is to return

-- | Subset type extracted from 'Plugins' to be sent to the IDE as
-- a description of the available commands
type IdePlugins = [(PluginId,[CommandDescriptor])]

instead of the full Plugins structure, which was returned before in the "base:plugins" command.

This does not touch the current commandDetail command that still returns the ExtendedCommandDescriptor.

[cocreature]

How about making that a map from PluginId to [CommandDescriptor]?

[alanz]

BTW, if I run the console (which in this case prints JSON encode output), I get

{"plugins":
  [
    ["base",
      [{"contexts":["none"],"name":"version","additional_params":[],"ui_description":"return HIE version","file_extensions":[]}
      ,{"contexts":["none"],"name":"plugins","additional_params":[],"ui_description":"list available plugins","file_extensions":[]}
      ,{"contexts":["none"],"name":"commands","additional_params":[{"rp":["plugin","the plugin name","text"]}],"ui_description":"list available commands for a given plugin","file_extensions":[]}
      ,{"contexts":["none"],"name":"commandDetail","additional_params":[{"rp":["plugin","the plugin name","text"]},{"rp":["command","the command name","text"]}],"ui_description":"list parameters required for a given command","file_extensions":[]}
      ]
    ]
   ,["eg2",
     [{"contexts":["none"],"name":"sayHello","additional_params":[],"ui_description":"say hello","file_extensions":[]}
     ,{"contexts":["none"],"name":"sayHelloTo","additional_params":[{"rp":["name","the name to greet","text"]}],"ui_description":"say hello to the passed in param","file_extensions":[]}
     ]
    ]
   ,["ghcmod",
     [{"contexts":["file"],"name":"check","additional_params":[],"ui_description":"check a file for GHC warnings and errors","file_extensions":[".hs",".lhs"]}
     ,{"contexts":["file"],"name":"lint","additional_params":[],"ui_description":"Check files using `hlint'","file_extensions":[".hs",".lhs"]}
     ,{"contexts":["project"],"name":"find","additional_params":[{"rp":["symbol","The SYMBOL to look up","text"]}],"ui_description":"List all modules that define SYMBOL","file_extensions":[".hs",".lhs"]}
     ,{"contexts":["file"],"name":"info","additional_params":[{"rp":["expr","The EXPR to provide info on","text"]}],"ui_description":"Look up an identifier in the context of FILE (like ghci's `:info')","file_extensions":[".hs",".lhs"]}
     ,{"contexts":["point"],"name":"type","additional_params":[],"ui_description":"Get the type of the expression under (LINE,COL)","file_extensions":[".hs",".lhs"]}
     ]
    ]
   ,["hare",
     [{"contexts":["point"],"name":"demote","additional_params":[],"ui_description":"Move a definition one level down","file_extensions":[".hs"]}
     ,{"contexts":["point"],"name":"dupdef","additional_params":[{"rp":["name","the new name","text"]}],"ui_description":"Duplicate a definition","file_extensions":[".hs"]}
     ,{"contexts":["region"],"name":"iftocase","additional_params":[],"ui_description":"Converts an if statement to a case statement","file_extensions":[".hs"]}
     ,{"contexts":["point"],"name":"liftonelevel","additional_params":[],"ui_description":"Move a definition one level up from where it is now","file_extensions":[".hs"]}
     ,{"contexts":["point"],"name":"lifttotoplevel","additional_params":[],"ui_description":"Move a definition to the top level from where it is now","file_extensions":[".hs"]}
     ,{"contexts":["point"],"name":"rename","additional_params":[{"rp":["name","the new name","text"]}],"ui_description":"rename a variable or type","file_extensions":[".hs"]}
     ]
    ]
   ]
  }

Which seems to do what we want. Maybe lose the outer layer "plugins":[] and do the list directly.

[cocreature]

I don’t see why we want an array of two element arrays here instead of a map from plugin names to an array of commands.

[alanz]

Because a PluginDescriptor is not round trip serialisable, because of the
CommandFunc.

And at the moment the [Service] layers are unused, and are intended for
internal plugin plumbing anyway.

So the only thing an IDE should care about is the PluginId and the
commands exposed, as per the CommandDescriptor.

On Mon, Nov 23, 2015 at 12:54 PM, Moritz Kiefer notifications@github.com
wrote:

How about making that a map from PluginId to [CommandDescriptor]?

—
Reply to this email directly or view it on GitHub
#93 (comment)
.

[alanz]

There is no reason not to do a map from PluginId to CommandDescriptor.

[cocreature]

I feel like we’re talking at cross purposes. I completely agree with the general idea of not serializing the PluginDescriptor. I just want it to be serialized as a map rather than an array of tuples, aka

{"plugins":
  {"base":
      [{"contexts":["none"],"name":"version","additional_params":[],"ui_description":"return HIE version","file_extensions":[]}
      ,{"contexts":["none"],"name":"plugins","additional_params":[],"ui_description":"list available plugins","file_extensions":[]}
      ,{"contexts":["none"],"name":"commands","additional_params":[{"rp":["plugin","the plugin name","text"]}],"ui_description":"list available commands for a given plugin","file_extensions":[]}
      ,{"contexts":["none"],"name":"commandDetail","additional_params":[{"rp":["plugin","the plugin name","text"]},{"rp":["command","the command name","text"]}],"ui_description":"list parameters required for a given command","file_extensions":[]}
      ]
   }
}

[gracjan]

Yes. A map please!

[cocreature]

LGTM, feel free to merge it. I’ll fix the elisp tests later.

Maybe we should separate the elisp part so that we don’t break travis everytime someone makes a change to the protocol and doesn’t also change the elisp.

Otherwise we’re going to end up with a state where we have five different IDE integrations in this repo and if someone makes a change he has to know each of them to be able to fix the tests.

[rvion]

Maybe we should separate the elisp part so that we don’t break travis everytime someone makes a change to the protocol and doesn’t also change the elisp.

@cocreature

I may be off, but I like the idea of plugins being in this repository.
I have the impression the more the community unite on this repo, the better it is.

Also, I'd much rather have a pull request delayed because it's breaking some ide plugins than the opposite. To my opinion, those failures are a good sign

[cocreature]

@rvion I tend to agree, but let me explain my reasoning:

At some point we’ll (hopefully) have at least integrations for emacs, vim & atom. I think it’s unreasonable to except contributors to know how the integrations in all three of those work. This puts us in a dilemma:

Either we force them to learn each of them, which doesn’t seem like a good idea. Or we allow people to submit prs with failing tests as long as they only concern IDE integrations and then wait for other people to contribute them and then merge all of those prs quickly to reduce the amount of breakage on master. I suppose we could also do prs against the branch of the pr with the new ide integrations, but I fear that we scare away contributors.

[rvion]

@cocreature in my opinion, I think it is better to either merge PR with some test failing and encourage the community to fix them soon after or let package maintainer make PR against specific repo branch to update their ide-integration code.

To my opinion, a failing test is the most effective way to make things get fixed. It's more effective than issue opening, more informative, and it invites all collaborators to improve the current state :)

Here are some more arguments in favor of a central unique repository with subfolders for different plugin integration:

• Hie was born because the community was too much divided. Splitting this package might destroy all of our hopes of unification. Imagine if some ide-integration-plugin authors decide to depends on an extra linux only tool ? We won't know right away, and we'll have no controll over it. If we want to end up with a single tool, I think we need to be host all ide plugins.
• Easier ide-plugins discoverability.
• easier to see share code/ideas between plugins, look at how one implemented such or such feature, and do the same for an other ide.
• One repository is the only simple way to ensure there is no regression or breakage across the ecosystem, to make stable releases, etc.
• I have the impression to often see ide-itegrators quickly become less motivated to update their plugins because it works good enough for them. Having them all here will federate more maintainers
• keeping everything together simplify future possible features, such as automating plugin installation directly in hie, etc..

ping @JPMoresmau @gracjan @alanz to include them in that discussion

[rvion]

@JPMoresmau @gracjan @alanz discussion starts here: #93 (comment)

[cocreature]

You convinced me :)