cvogt / cbt

CBT - fun, fast, intuitive, compositional, statically checked builds written in Scala
Other
488 stars 60 forks source link

Add support for generating ensime configuration file #539

Closed jodersky closed 7 years ago

jodersky commented 7 years ago

This plugin adds the capability to generate ENSIME configuration files. It's settings are mostly copied from sbt-ensime.

Currently, only configuration for single-project builds is generated. I'll need to look deeper into how I can list a build's DirectoryDependencies, in order add support for multi-module projects.

Closes #89

FYI @fommil

jodersky commented 7 years ago

oooh, I just saw #511. It might help to get multi-module builds working

fommil commented 7 years ago

Nice! Don't forget to include the NOTICE file from ensime-sbt to stay compliant with the license.

Is there any way to add tests in cbt yet, like sbt scripted?

jodersky commented 7 years ago

@fommil, I'm not sure how a NOTICE file is usually structured. Is the one I added acceptable?

Regarding tests, we can probably add a test project to /tests or in a sub directory of /stage2/plugins. @cvogt what's the preferred way of adding tests to plugins?

cvogt commented 7 years ago

Woo, this is really awesome :D.

Regarding tests. Right now everything is triggers from test/test.scala. For plugins there are usually examples in examples/ensime/ and those can serve as test cases. You can add something like this towards the end of the file:

    {
      val res = runCbt("../examples/ensime", Seq("ensime"))
      assert(res.exit0)
      assert(res.out contains "<some sexp stuff maybe?>", res.out)
    }

checking for some substring might give a good level of confidence already? Wouldn't validate the details obviously.

Regarding the implementation of the plugin. Very clean, I like it. Early this year I started using a slightly different plugin style using slightly more unconventional Scala, but solving the problem of the ever-growing number of top-level name-prefixed methods in the Build class body. Instead of having ensimeScalaJars a top-level, public method, you could inline it or make it private. Things can still be configured.

Instead of

override def ensimeScalaJars = super. ensimeScalaJars :+ ...

one would write

override def ensimeConfig = super. ensimeConfig.copy(
  scalaJars = super. ensimeConfig.scalaJars :+ ...
)

Also instead of having both ensimeConfig and ensime, I started having only one key, i.e. ensime, which is the config case class instance, but giving it an apply method, that then actually does whatever is supposed to happen, i.e. generate the ensime config file. Here is an example: https://github.com/cvogt/cbt/commit/435267714fd9be710945d5d6c2d0ee1f18294e12#diff-66df9d0cbaa60c09eb94d2865a239462

I use nested case classes. (Outer one for non-overridable config, inner one for overridable config, inner one's .apply to execute things). This will certainly be slightly unfamiliar, but it does match the problem nicely. Do you guys think that's trying to be too smart and we should rather stick rather to the style @jodersky's ensime plugin chose? I would like to standardize on one style across all plugins. The disadvantage of the current ensime plugins style is mainly that every plugin will basically have to name-prefix all of their top-level keys to avoid conflicts with other plugins. The advantage is it is more familiar (e.g. more how sbt does things) and doesn't use nested classes, so fewer of the language. Opinions?

jodersky commented 7 years ago

I like the style you propose, @cvogt. It keeps things very focused and also avoids unused declarations in case someone overrides the top-level task. I.e. in the current situation, if someones overrides ensime, then all ensimeXXX config methods will most likely become useless.

I must admit that I barely looked through other plugins before starting work on this one. I'll refactor this plugin to take advantage of #511 and will try using the style you propose

jodersky commented 7 years ago

@cvogt, I updated the plugin to support multi-project builds (i.e.builds with other BaseBuild dependencies) and consolidated some tasks as per our gitter discussion. Unfortunately, I was not able to fold all settings into a single public method. Here is my rationale for the various public methods:

  1. ensime. This is the plugin's main method.
  2. ensimeConfig. This does what a nested config case class would do in the design you propose. Now, I could create a dummy apply#config class with identical fields as EnsimeConfig, however that seems kind of redundant. Another approach would be to nest the whole plugin companion object in an apply case class. I am however not sure if such a nesting is the simplest approach, since the utilities may be useful by themselves as well.
  3. ensimeServerVersion. I added this as a top-level method since many ensime config values depend on it. Since this plugin is made to work only with the latest ensime version, and since it is currently a -SNAPSHOT unstable version, I think it is safer to define it as an independent def rather than copy its value into multiple internal config values.

Please let me know if you have any suggestions to combine the 3 tasks, especially 1 and 2.

jodersky commented 7 years ago

I removed the explicit ensime server version setting in the latest commit. The caveat is two-fold:

  1. users will have to make sure that ensimeServerVersion and ensimeServerJars are coherent when customizing either one of the settings
  2. at least java 1.8 is required
jodersky commented 7 years ago

Check out this diff to see a prototype with a single ensime method. I'm really not sure if the added complexity is worth it

cvogt commented 7 years ago

happy to merge as is unless you want to do some adjustments?

jodersky commented 7 years ago

I've been pondering different approaches on how to unify all settings in one public method but none seem clean so far. The biggest issue I have with wrapping EnsimeConfig in an apply is that it will suddenly be tied to a parent class instance, although it doesn't need it for its functionality. One way to work around that would be to duplicate a config class within apply, that's not very DRY however

So in the mean time, yes please go ahead and merge! :)