Merge pull request #23 from ShaneDelmore/add_advice_from_resources

Add advice from resources

Author: adamw

README.md

@@ -61,7 +61,6 @@ Then create a file named .clippy.json in the root of your project directory and
   "version": "0.3.1",
   "advices": [
     {
-      "id": 2,
       "error": {
         "type": "typeMismatch",
         "found": "scala\\.concurrent\\.Future\\[Int\\]",
@@ -78,6 +77,15 @@ Then create a file named .clippy.json in the root of your project directory and
 }
 ````
 
+# Library specific advice
+
+If you have advice that is specific to a library or library version you can also bundle the advice with your library.
+If your users have Scala-Clippy installed they will see your advice if your library is inclued in their project.
+This can be helpful in the common case where users of your library need specific imports to be able to use your functionality.
+To bundle clippy advice with your library just put it in a file named clippy.json in your resources directory
+
+For examples on how to write tests for advice to ensure it does not go out of date see [CompileTests.scala](./tests/src/test/scala/org/softwaremill/clippy/CompileTests.scala)
+
 # Alternative ways to use Clippy
 
 You can also use Clippy directly as a compiler plugin. If you use SBT, add the following setting to your

model/src/main/scala/com/softwaremill/clippy/Advice.scala

@@ -2,13 +2,12 @@ package com.softwaremill.clippy
 
 import org.json4s.JsonAST._
 
-case class Advice(id: Long, compilationError: CompilationError[RegexT], advice: String, library: Library) {
+case class Advice(compilationError: CompilationError[RegexT], advice: String, library: Library) {
   def errMatching: PartialFunction[CompilationError[ExactT], String] = {
     case ce if compilationError.matches(ce) => advice
   }
 
   def toJson: JValue = JObject(
-      "id" -> JInt(id),
       "error" -> compilationError.toJson,
       "text" -> JString(advice),
       "library" -> library.toJson
@@ -19,12 +18,11 @@ object Advice {
   def fromJson(jvalue: JValue): Option[Advice] = {
     (for {
       JObject(fields) <- jvalue
-      JField("id", JInt(id)) <- fields
       JField("error", errorJV) <- fields
       error <- CompilationError.fromJson(errorJV).toList
       JField("text", JString(text)) <- fields
       JField("library", libraryJV) <- fields
       library <- Library.fromJson(libraryJV).toList
-    } yield Advice(id.longValue(), error, text, library)).headOption
+    } yield Advice(error, text, library)).headOption
   }
 }

plugin/src/main/scala/com/softwaremill/clippy/AdviceLoader.scala

@@ -10,22 +10,30 @@ import scala.concurrent.{ExecutionContext, Future}
 import scala.io.Source
 import scala.tools.nsc.Global
 import scala.util.{Failure, Success, Try}
+import scala.collection.JavaConversions._
 
 class AdviceLoader(global: Global, url: String, localStoreDir: File, projectAdviceFile: Option[File])(implicit ec: ExecutionContext) {
   private val OneDayMillis = 1000L * 60 * 60 * 24
 
   private val localStore = new File(localStoreDir, "clippy.json.gz")
 
+  private val resourcesAdvice: List[Advice] =
+    getClass.getClassLoader
+      .getResources("clippy.json")
+      .toList
+      .flatMap(loadAdviceFromUrL)
+
+  private def loadAdviceFromUrL(url: URL): List[Advice] =
+    TryWith(url.openStream())(inputStreamToClippy(_).advices) match {
+      case Success(advices) => advices
+      case Failure(_) =>
+        global.warning(s"Cannot load advice from ${url.getPath} : Ignoring.")
+        Nil
+    }
+
   private val projectAdvice: List[Advice] =
-    projectAdviceFile.flatMap { file =>
-      Try(loadLocally(file))
-        .map(bytes => inputStreamToClippy(decodeUtf8Bytes(bytes)).advices)
-        .recover {
-          case e: Exception =>
-            global.warning(s"Cannot load advice from project store: $file. Ignoring.")
-            throw e
-        }.toOption
-    }.getOrElse(Nil)
+    projectAdviceFile.map(file =>
+      loadAdviceFromUrL(file.toURI.toURL)).getOrElse(Nil)
 
   def load(): Future[Clippy] = {
     val localClippy = if (!localStore.exists()) {
@@ -52,8 +60,9 @@ class AdviceLoader(global: Global, url: String, localStoreDir: File, projectAdvi
       }
     }
 
-    // Add in project specific advice
-    localClippy.map(clippy => clippy.copy(advices = projectAdvice ++ clippy.advices))
+    // Add in advice found in resources and project root
+    localClippy.map(clippy =>
+      clippy.copy(advices = (projectAdvice ++ resourcesAdvice ++ clippy.advices).distinct))
   }
 
   private def fetchStoreParse(): Future[Clippy] =
@@ -99,8 +108,7 @@ class AdviceLoader(global: Global, url: String, localStoreDir: File, projectAdvi
     if (!localStoreDir.isDirectory && !localStoreDir.mkdir()) {
       throw new IOException(s"Cannot create directory $localStoreDir")
     }
-    val os = new FileOutputStream(localStore)
-    try os.write(bytes) finally os.close()
+    TryWith(new FileOutputStream(localStore))(_.write(bytes)).get
   }
 
   private def storeLocallyInBackground(bytes: Array[Byte]): Unit = {

plugin/src/main/scala/com/softwaremill/clippy/ClippyPlugin.scala

@@ -28,10 +28,18 @@ class ClippyPlugin(val global: Global) extends Plugin {
     global.reporter = new DelegatingReporter(r, handleError)
 
     def handleError(pos: Position, msg: String): String = {
-      val totalMatchingFunction = advices.map(_.errMatching)
-        .foldLeft(PartialFunction.empty[CompilationError[ExactT], String])(_.orElse(_)).lift
-      val adviceText = CompilationErrorParser.parse(msg).flatMap(totalMatchingFunction).map("\n Clippy advises: " + _).getOrElse("")
-      msg + adviceText
+      val parsedMsg = CompilationErrorParser.parse(msg)
+      val matchers = advices.map(_.errMatching.lift)
+      val matches = matchers.flatMap(pf => parsedMsg.flatMap(pf))
+
+      matches.size match {
+        case 0 =>
+          msg
+        case 1 =>
+          matches.mkString(s"$msg\n Clippy advises: ", "", "")
+        case _ =>
+          matches.mkString(s"$msg\n Clippy advises you to try one of these solutions: \n   ", "\n or\n   ", "")
+      }
     }
   }
 

plugin/src/main/scala/com/softwaremill/clippy/Utils.scala

@@ -1,6 +1,10 @@
 package com.softwaremill.clippy
 
 import java.io.{ByteArrayOutputStream, InputStream}
+import java.io.Closeable
+import scala.util.control.NonFatal
+import scala.util.{Failure, Try}
+
 
 object Utils {
   /**
@@ -41,4 +45,26 @@ object Utils {
     }
     finally is.close()
   }
+
+  object TryWith {
+    def apply[C <: Closeable, R](resource: => C)(f: C => R): Try[R] =
+      Try(resource).flatMap(resourceInstance => {
+        try {
+          val returnValue = f(resourceInstance)
+          Try(resourceInstance.close()).map(_ => returnValue)
+        }
+        catch {
+          case NonFatal(exceptionInFunction) =>
+            try {
+              resourceInstance.close()
+              Failure(exceptionInFunction)
+            }
+            catch {
+              case NonFatal(exceptionInClose) =>
+                exceptionInFunction.addSuppressed(exceptionInClose)
+                Failure(exceptionInFunction)
+            }
+        }
+      })
+  }
 }

tests/src/test/scala/org/softwaremill/clippy/CompileTests.scala

@@ -6,6 +6,7 @@ import java.util.zip.GZIPOutputStream
 import com.softwaremill.clippy._
 import org.scalatest.{BeforeAndAfterAll, Matchers, FlatSpec}
 
+import scala.tools.reflect.ToolBox
 import scala.tools.reflect.ToolBoxError
 
 class CompileTests extends FlatSpec with Matchers with BeforeAndAfterAll {
@@ -26,31 +27,32 @@ class CompileTests extends FlatSpec with Matchers with BeforeAndAfterAll {
 
     val advices = List(
       Advice(
-        1L,
         TypeMismatchError(ExactT("slick.dbio.DBIOAction[*]"), None, ExactT("slick.lifted.Rep[Option[*]]"), None).asRegex,
         "Perhaps you forgot to call .result on your Rep[]? This will give you a DBIOAction that you can compose with other DBIOActions.",
         Library("com.typesafe.slick", "slick", "3.1.0")
       ),
       Advice(
-        2L,
         TypeMismatchError(ExactT("akka.http.scaladsl.server.StandardRoute"), None, ExactT("akka.stream.scaladsl.Flow[akka.http.scaladsl.model.HttpRequest,akka.http.scaladsl.model.HttpResponse,Any]"), None).asRegex,
         "did you forget to define an implicit akka.stream.ActorMaterializer? It allows routes to be converted into a flow. You can read more at http://doc.akka.io/docs/akka-stream-and-http-experimental/2.0/scala/http/routing-dsl/index.html",
         Library("com.typesafe.akka", "akka-http-experimental", "2.0.0")
       ),
       Advice(
-        3L,
         NotFoundError(ExactT("value wire")).asRegex,
         "you need to import com.softwaremill.macwire._",
         Library("com.softwaremill.macwire", "macros", "2.0.0")
       ),
       Advice(
-        4L,
+        NotFoundError(ExactT("value wire")).asRegex,
+        "If you need further help check out the macwire readme at https://github.com/adamw/macwire",
+        Library("com.softwaremill.macwire", "macros", "2.0.0")
+      ),
+      Advice(
         TypeArgumentsDoNotConformToOverloadedBoundsError(
-          ExactT("*"), ExactT("value apply"), Set(
-            ExactT("[E <: slick.lifted.AbstractTable[_]]=> slick.lifted.TableQuery[E]"),
-            ExactT("[E <: slick.lifted.AbstractTable[_]](cons: slick.lifted.Tag => E)slick.lifted.TableQuery[E]")
-          )
-        ).asRegex,
+        ExactT("*"), ExactT("value apply"), Set(
+          ExactT("[E <: slick.lifted.AbstractTable[_]]=> slick.lifted.TableQuery[E]"),
+          ExactT("[E <: slick.lifted.AbstractTable[_]](cons: slick.lifted.Tag => E)slick.lifted.TableQuery[E]")
+        )
+      ).asRegex,
         "incorrect class name passed to TableQuery",
         Library("com.typesafe.slick", "slick", "3.1.1")
       )
@@ -110,24 +112,26 @@ class CompileTests extends FlatSpec with Matchers with BeforeAndAfterAll {
                  """.stripMargin
   )
 
+  val tb = {
+    val cpp = sys.env("CLIPPY_PLUGIN_PATH")
+
+    import scala.reflect.runtime._
+    val cm = universe.runtimeMirror(getClass.getClassLoader)
+
+    cm.mkToolBox(options = s"-Xplugin:$cpp -Xplugin-require:clippy")
+  }
+
+  def parse(snippet: String) = tb.eval(tb.parse(snippet))
+
   for ((name, s) <- snippets) {
     name should "compile with errors" in {
-      val cpp = sys.env("CLIPPY_PLUGIN_PATH")
-
-      import scala.reflect.runtime._
-      val cm = universe.runtimeMirror(getClass.getClassLoader)
-
-      import scala.tools.reflect.ToolBox
-      val tb = cm.mkToolBox(options = s"-Xplugin:$cpp -Xplugin-require:clippy")
-
-      try {
-        tb.eval(tb.parse(s))
-        fail("Should report compile errors")
-      }
-      catch {
-        case e: ToolBoxError =>
-          e.message should include("Clippy advises")
-      }
+      (the[ToolBoxError] thrownBy parse(s)).message should include("Clippy advises")
     }
   }
+
+  "Clippy" should "return all matching advice" in {
+    (the[ToolBoxError] thrownBy parse(snippets("macwire")))
+      .message should include("Clippy advises you to try one of these")
+  }
+
 }

ui/app/dal/AdvicesRepository.scala

@@ -54,7 +54,7 @@ class AdvicesRepository(database: SqlDatabase, idGenerator: IdGenerator)(implici
 case class StoredAdvice(id: Long, errorTextRaw: String, patternRaw: String, compilationError: CompilationError[RegexT],
     advice: String, state: AdviceState, library: Library, contributor: Contributor, comment: Option[String]) {
 
-  def toAdvice = Advice(id, compilationError, advice, library)
+  def toAdvice = Advice(compilationError, advice, library)
   def toAdviceListing = AdviceListing(id, compilationError, advice, library,
     ContributorListing(contributor.github, contributor.twitter))
 }