Upgrade to scalameta 3.7.0 with default settings. #675

olafurpg · 2018-04-04T18:46:36Z

This commit is a major milestone in that Scalafix now uses the latest
SemanticDB with default settings. Previously, scalafix relied on two
features that are now no longer default:

-P:semanticdb:denotations:all, to lookup information about any symbol
scalafix relied on semanticdb-scalac to persist full information for
all referenced symbols in each compilation unit. Now, scalafix
can load information directly from the classpath thanks to metacp
and the new --dependency-classpath command line flag.
-P:semanticdb:signatures:old, these were pretty-printed versions of
types that have been replaced with the new specification of Type in
https://github.com/scalameta/scalameta/blob/master/semanticdb/semanticdb3/semanticdb3.md#type

There's quite a bit happening in this PR, it's roughly broken down by:

New --dependency-classpath cli flag be processed by metacp.
New SymbolTable to lookup information about symbols from the metacp
processed classpath.
Refactor ExplicitResultTypes to use s.Type instead of the deprecated
s.SymbolInformation.signatur
Remove tests for Symbol.Multi, which are no longer emitted by
semanticdb-scalac.

These changes required updates to testkit, the cli and sbt-scalafix.

Future work:

The tests pass but I noticed a couple errors while running
ExplicitResultTypes on Slick. Given that this PR already does quite a
lot I prefer to get it merged asap and fix those errors later.
ExplicitResultTypes crashes on java paths #674
--dependency-classpath may not be necessary. We might be able to away
with only --classpath and be smarter about which files to load from
the provided files to fix.

This commit is a major milestone in that Scalafix now uses the latest SemanticDB with default settings. Previously, scalafix relied on two features that are now no longer default: * -P:semanticdb:denotations:all, to lookup information about any symbol scalafix relied on semanticdb-scalac to persist full information for all referenced symbols in each compilation unit. Now, scalafix can load information directly from the classpath thanks to metacp and the new --dependency-classpath command line flag. * -P:semanticdb:signatures:old, these were pretty-printed versions of types that have been replaced with the new specification of `Type` in https://github.com/scalameta/scalameta/blob/master/semanticdb/semanticdb3/semanticdb3.md#type There's quite a bit happening in this PR, it's roughly broken down by: * New --dependency-classpath cli flag be processed by metacp. * New `SymbolTable` to lookup information about symbols from the metacp processed classpath. * Refactor ExplicitResultTypes to use s.Type instead of the deprecated s.SymbolInformation.signatur * Remove tests for Symbol.Multi, which are no longer emitted by semanticdb-scalac. These changes required updates to testkit, the cli and sbt-scalafix. Future work: * The tests pass but I noticed a couple errors while running ExplicitResultTypes on Slick. Given that this PR already does quite a lot I prefer to get it merged asap and fix those errors later. * --dependency-classpath may not be necessary. We might be able to away with only --classpath and be smarter about which files to load from the provided files to fix.

ShaneDelmore · 2018-04-04T19:47:22Z

build.sbt

+lazy val scalafixSbt1 = scalafixSbt(
+  scala212,
+  sbt1,
+  _.dependsOn(testUtils212 % Test)


Was this formatting change intended?

Yes, for consistency with the changes below

ShaneDelmore · 2018-04-04T19:51:09Z

scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala

-    def resolveClasspath: Configured[Classpath] =
-      classpath match {
+    def resolveClasspath: Configured[Classpath] = {
+      classDirectory match {


Not a blocker but I don't like that this name hints that the classpath is a single directory entry, which is not what I usually see for a classpath.

I agree this is confusing. Note that the cli still accepts --classpath as an alias for --class-directory. I renamed for consistency with sbt classDirectory to distinguish it from dependencyClasspath.

The public facing API is fully compatible with the previous behavior despite this change so we have wiggle room to improve on this in the future.

xeno-by · 2018-04-04T20:41:53Z

scalafix-cli/src/main/scala/scalafix/internal/cli/ClasspathOps.scala

+    * @param sclasspath Regular classpath to process.
+    * @param out The output stream to print out error messages.
+    */
+  def toMetaClasspath(sclasspath: Classpath, out: PrintStream): Classpath = {


The fact that these methods take a printstream looks really awkward.

awkward sure, but what do you propose instead? This is not a public API

Use System.out directly.

Scalafix runs via nailgun, in IDEs with custom logging, etc,

It's not OK to use System.out

xeno-by · 2018-04-04T20:42:30Z

scalafix-cli/src/main/scala/scalafix/internal/cli/ClasspathOps.scala

+      .withScalaLibrarySynthetics(true)
+    val reporter = metacp.Reporter().withOut(out)
+    val mclasspath = scala.meta.cli.Metacp.process(settings, reporter).get
+    mclasspath


Is it okay to crash if metacp returns None?

I've refactored the code to propagate the error and report a helpful message on failure.

xeno-by · 2018-04-04T20:43:18Z

scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala

@@ -55,12 +55,20 @@ case class ScalafixOptions(
    sourceroot: Option[String] = None,
    @HelpMessage(
      "java.io.File.pathSeparator separated list of directories or jars containing " +


Is this a list or a single directory?

It remains a list, unchanged from the previous behavior of --classpath. I've renamed it back to --classpath and added more details about --dependency-classpath.

xeno-by · 2018-04-04T20:45:16Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/CombinedSymbolTable.scala

+  * This utility is necessary because SymbolTable contains only global symbols and
+  * SemanticdbIndex has local symbols.
+  */
+class CombinedSymbolTable(index: SemanticdbIndex, table: SymbolTable) {


Why do we need to pass both index and table here, given that EagerInMemorySemanticdbIndex already takes a symbol table?

Good catch, I've merged this into EagerSemanticdbIndex avoiding the need for CombinedSymbolTable.

xeno-by · 2018-04-04T20:46:18Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/LazySymbolTable.scala

+class LazySymbolTable(mclasspath: Classpath) extends SymbolTable {
+
+  // Map from symbols to the paths to their corresponding semanticdb file where they are stored.
+  private val unloadedSymbols = TrieMap.empty[String, AbsolutePath]


"unloaded" as in "not yet loaded" or as in "previously loaded, but no longer available"?

Renamed to notYetLoadedSymbols

xeno-by · 2018-04-04T20:49:25Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/LazySymbolTable.scala

+    val fs = newJarFileSystem(path)
+    // NOTE(olafur): We don't fs.close() because that can affect another place where `FileSystems.getFileSystems`
+    // was used due to a `FileSystemAlreadyExistsException`. I don't know what the best solution is for reading the
+    // same zip file from multiple concurrent places.


Maybe this is/was the source of deadlocks?

That was my first suspicion as well! I continuously ran the test suite to see problems. This wasn't the cause of the deadlock, I checked the stack traces and they led to metacp.

xeno-by · 2018-04-04T20:50:39Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

+      shortenNames: Boolean,
+      pos: Position)(implicit index: SemanticdbIndex): Option[(Type, Patch)] = {
+    try {
+      val table = index.asInstanceOf[EagerInMemorySemanticdbIndex].table


Woah, this cast looks scary.

Seconded, why do we know more than the compiler here?

The SemanticdbIndex trait should only have a single implementation, I've kept the abstract trait to separate the public API from the implementation. I admit this is not an ideal situation but the alternative is to expose implementation details in the public API. We have the same situation in other places, for example for linter suppression. private[scalafix] does not bite enough IMO compared to forcing a cast.

xeno-by · 2018-04-04T20:53:14Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

+  /*
+   * Returns a scala.meta.Tree given a scala.meta.Symbol.
+   *
+   * NOTE: this method has become an utter mess and is in need of a clean reimplementation.


Okay, not gonna review then.

The implementation is unchanged from the previous TypeSyntax, the diff is mostly reformatting with minor changes.

ShaneDelmore · 2018-04-04T21:02:30Z

scalafix-core/shared/src/main/scala/scalafix/internal/rule/ExplicitResultTypes.scala

+        result <- TypeSyntax.prettify(
+          tpe,
+          ctx,
+          config.unsafeShortenNames,


How is this config passed? https://scalacenter.github.io/scalafix/docs/users/configuration shows how to add rules to the scalafix config, but does not show if it is possible to pass rule specific config.

It's configuration for ExplicitResultTypes documented here https://scalacenter.github.io/scalafix/docs/rules/ExplicitResultTypes

I wasn’t able to put that together from the docs for some reason but I get it now, thanks.

ShaneDelmore · 2018-04-04T21:03:10Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/CombinedSymbolTable.scala

+  * This utility is necessary because SymbolTable contains only global symbols and
+  * SemanticdbIndex has local symbols.
+  */
+class CombinedSymbolTable(index: SemanticdbIndex, table: SymbolTable) {


Is combined symbol table needed if shortenNames is false?

It's needed both when shortenNames is true and false.

ShaneDelmore · 2018-04-04T21:09:10Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/LazySymbolTable.scala

+  private val unloadedSymbols = TrieMap.empty[String, AbsolutePath]
+  private val loadedSymbols = TrieMap.empty[String, s.SymbolInformation]
+  private val semanticdb = "META-INF/semanticdb"
+  private val semanticIdx = "META-INF/semanticdb.semanticidx"


would this be a good place to add

private val manifest = "META-INF/MANIFEST.MF"

in case there is a classpath manifest in a jar?

I think that should be handled elsewhere, for example scala.meta.Classpath should be able to handle.

ShaneDelmore · 2018-04-04T21:11:07Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/LazySymbolTable.scala

+    if (root.isDirectory) {
+      loadIndex(root, Files.newInputStream(root.resolve(semanticIdx).toNIO))
+    } else if (PathIO.extension(root.toNIO) == "jar") {
+      withJarFileSystem(root) { jarRoot =>


I am not familiar enough with the NIO api to know the answer to this, but are you sure that these apis handle symlinks?

ShaneDelmore · 2018-04-04T22:59:14Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

 import org.langmeta.semanticdb.Symbol
+import scala.meta.internal.semanticdb3.SingletonType.{Tag => x}


This saves two characters and makes the code harder to read. Why not just Tag.?

I've replaced x with Tag but kept the import local, Tag is used in s.Type.Tag/s.SingletonType.Tag and a few others I think.

ShaneDelmore · 2018-04-04T23:15:06Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

+  }
+
+  def isFunctionN(symbol: String): Boolean = {
+    symbol.startsWith("scala.Function") &&


I don't know if there this is a perforamance issue, but from an accuracy issue how about

"scala.Function(\\d)+#".r

to make sure we capture Function1 but not FunctionPure (not that such a thing currently exists).

there are no other scala.Function* symbols so I thought this would be enough, regexes are such a heavy machine.

ShaneDelmore · 2018-04-04T23:27:33Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

    }
+    symbol match {
+      case Symbol.Global(owner, Signature.Term(_) | Signature.Type(_)) =>


I read this as "A symbol is stable only if it is A Type or Term owned by Symbol.None or owned by a chain of n Global Terms". Is that correct?

This method suffers from the same problems as symbolToTree, it can be ignored.

With the introduction of SymbolInformation one should no longer infer any information from the structure of symbols. Symbols are purely an ID and nothing more.

ShaneDelmore · 2018-04-04T23:34:57Z

...-tests/output/src/main/scala/test/explicitResultTypes/ExplicitResultTypesPathDependent.scala

@@ -4,7 +4,7 @@ object ExplicitResultTypesPathDependent {
  class Path {
    class B { class C }
    implicit val x: Path.this.B = new B
-    implicit val y: x.C = new x.C
+    implicit val y: Path.this.x.C = new x.C
    def gimme(yy: x.C) = ???; gimme(y)


??? indeed.

ShaneDelmore

I had a few questions but don't have enough familiarity with the codebase to call any of them a blocker.

* revert --class-directory, stick to --classpath * better error handling for metacp interaction * add --metacp-cache-directory * add --metacp-no-par for pending --par flag in metacp

olafurpg

Thank you for the review! I've addressed all comments, but I'm still investigating the CI failures.

olafurpg · 2018-04-05T10:13:23Z

build.sbt

+lazy val scalafixSbt1 = scalafixSbt(
+  scala212,
+  sbt1,
+  _.dependsOn(testUtils212 % Test)


Yes, for consistency with the changes below

olafurpg · 2018-04-05T10:16:21Z

scalafix-cli/src/main/scala/scalafix/cli/CliRunner.scala

-    def resolveClasspath: Configured[Classpath] =
-      classpath match {
+    def resolveClasspath: Configured[Classpath] = {
+      classDirectory match {


I agree this is confusing. Note that the cli still accepts --classpath as an alias for --class-directory. I renamed for consistency with sbt classDirectory to distinguish it from dependencyClasspath.

The public facing API is fully compatible with the previous behavior despite this change so we have wiggle room to improve on this in the future.

olafurpg · 2018-04-05T10:16:51Z

scalafix-cli/src/main/scala/scalafix/internal/cli/ClasspathOps.scala

+    * @param sclasspath Regular classpath to process.
+    * @param out The output stream to print out error messages.
+    */
+  def toMetaClasspath(sclasspath: Classpath, out: PrintStream): Classpath = {


awkward sure, but what do you propose instead? This is not a public API

olafurpg · 2018-04-05T10:19:55Z

scalafix-cli/src/main/scala/scalafix/internal/cli/ClasspathOps.scala

+      .withScalaLibrarySynthetics(true)
+    val reporter = metacp.Reporter().withOut(out)
+    val mclasspath = scala.meta.cli.Metacp.process(settings, reporter).get
+    mclasspath


I've refactored the code to propagate the error and report a helpful message on failure.

olafurpg · 2018-04-05T10:59:38Z

scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala

@@ -55,12 +55,20 @@ case class ScalafixOptions(
    sourceroot: Option[String] = None,
    @HelpMessage(
      "java.io.File.pathSeparator separated list of directories or jars containing " +


It remains a list, unchanged from the previous behavior of --classpath. I've renamed it back to --classpath and added more details about --dependency-classpath.

olafurpg · 2018-04-05T11:23:15Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

+      shortenNames: Boolean,
+      pos: Position)(implicit index: SemanticdbIndex): Option[(Type, Patch)] = {
+    try {
+      val table = index.asInstanceOf[EagerInMemorySemanticdbIndex].table


The SemanticdbIndex trait should only have a single implementation, I've kept the abstract trait to separate the public API from the implementation. I admit this is not an ideal situation but the alternative is to expose implementation details in the public API. We have the same situation in other places, for example for linter suppression. private[scalafix] does not bite enough IMO compared to forcing a cast.

olafurpg · 2018-04-05T11:23:56Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

+  }
+
+  def isFunctionN(symbol: String): Boolean = {
+    symbol.startsWith("scala.Function") &&


there are no other scala.Function* symbols so I thought this would be enough, regexes are such a heavy machine.

olafurpg · 2018-04-05T11:24:55Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

    }
+    symbol match {
+      case Symbol.Global(owner, Signature.Term(_) | Signature.Type(_)) =>


This method suffers from the same problems as symbolToTree, it can be ignored.

olafurpg · 2018-04-05T11:25:33Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

    }
+    symbol match {
+      case Symbol.Global(owner, Signature.Term(_) | Signature.Type(_)) =>


With the introduction of SymbolInformation one should no longer infer any information from the structure of symbols. Symbols are purely an ID and nothing more.

olafurpg · 2018-04-05T11:26:20Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

+  /*
+   * Returns a scala.meta.Tree given a scala.meta.Symbol.
+   *
+   * NOTE: this method has become an utter mess and is in need of a clean reimplementation.


The implementation is unchanged from the previous TypeSyntax, the diff is mostly reformatting with minor changes.

Tests were failing with `java.nio.file.NoSuchFileException: /META-INF/semanticdb.semanticidx` on 2.11, but not 2.12. I was unable to reproduce the problem using `JarFile` from java.io so I'm gonna throw in the towel and use JarFile for reading zip files instead.

The Windows CI was failing due to URI encoding issues: ``` [info] scalafix.tests.rule.SemanticTests *** ABORTED *** (0 milliseconds) [info] java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/projects/scalafix/.cross/unit/target/scala-2.12/test-classes/ ``` My experience is that this is typically caused by stringly typed programming around URIs, so let's hope this commit fixes the issue.

olafurpg · 2018-04-05T14:50:49Z

I'm investigating the CI build failures in scalameta/scalameta#1476 I suspect they are caused by even more incorrect usage of nio FileSystems.

The Travis 2.11 failures seem to be caused by differences is positions compared to 3.2.0

Semanticdb-scala no longer emits accurate dialects so we need to use other tricks to compute the scala version specific directory

xeno-by · 2018-04-08T22:43:15Z

scalafix-cli/src/main/scala/scalafix/internal/cli/ClasspathOps.scala

+      .withCacheDir(cacheDirectory.getOrElse(default.cacheDir))
+    val reporter = metacp
+      .Reporter()
+      .withOut(devNull) // out prints classpath of proccessed classpath, which is not relevant for scalafix.


Huh, this is unfortunate, given that we're calling process directly.

xeno-by · 2018-04-08T22:44:56Z

scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala

+        "On macOS the default cache directory is ~/Library/Caches/semanticdb. ")
+    metacpCacheDir: Option[String] = None,
+    @HelpMessage("Set this flag to disable parallel processing with metacp.")
+    metacpNoPar: Boolean = false,


I think that the help message should explain that there is a danger of deadlocks.

xeno-by · 2018-04-08T22:45:23Z

scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala

+        "On macOS the default cache directory is ~/Library/Caches/semanticdb. ")
+    metacpCacheDir: Option[String] = None,
+    @HelpMessage("Set this flag to disable parallel processing with metacp.")
+    metacpNoPar: Boolean = false,


@ShaneDelmore We'll need to set this flag when we'll be upgrading Scalafix internally.

xeno-by · 2018-04-08T22:49:15Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

+  }
+}
+
+class TypeSyntax private (


Would you ultimately want parts of this code to end up in Metap? scalameta/scalameta#1479

I'd like to rewrite this file from scratch first before considering that. I am not happy with the code quality here, but I am more eager to upgrade to scalameta v3.7 and this PR is already doing too much.

This should fix the CI error on Windows

olafurpg

Thanks for the review! Upgrading to scalameta v3.7.2 fixed the last blocking problem on Windows

olafurpg · 2018-04-09T10:58:55Z

scalafix-cli/src/main/scala/scalafix/internal/cli/ScalafixOptions.scala

+        "On macOS the default cache directory is ~/Library/Caches/semanticdb. ")
+    metacpCacheDir: Option[String] = None,
+    @HelpMessage("Set this flag to disable parallel processing with metacp.")
+    metacpNoPar: Boolean = false,


olafurpg · 2018-04-09T10:59:59Z

scalafix-core/shared/src/main/scala/scalafix/internal/util/TypeSyntax.scala

+  }
+}
+
+class TypeSyntax private (


I'd like to rewrite this file from scratch first before considering that. I am not happy with the code quality here, but I am more eager to upgrade to scalameta v3.7 and this PR is already doing too much.

olafurpg · 2018-04-09T12:43:15Z

Merging and releasing 0.6.0-M2

Note that ExplicitResultTypes in M2 will be a fairly broken until TypeSyntax gets a rewrite that I'm planning for this week.

olafurpg requested a review from ShaneDelmore April 4, 2018 18:46

Don't read semanticdb index if it doesn't exist

7c733f0

ShaneDelmore reviewed Apr 4, 2018

View reviewed changes

xeno-by suggested changes Apr 4, 2018

View reviewed changes

ShaneDelmore reviewed Apr 4, 2018

View reviewed changes

Address review feedback.

9dcf454

* revert --class-directory, stick to --classpath * better error handling for metacp interaction * add --metacp-cache-directory * add --metacp-no-par for pending --par flag in metacp

olafurpg commented Apr 5, 2018

View reviewed changes

olafurpg added 3 commits April 5, 2018 15:25

Provide a more helpful error message when cli tests fail.

f3653e4

olafurpg mentioned this pull request Apr 5, 2018

Upgrade to Scalameta v4.0 scalameta/metals#239

Closed

3 tasks

Fix test failure in 2.11.

7845d60

Semanticdb-scala no longer emits accurate dialects so we need to use other tricks to compute the scala version specific directory

xeno-by approved these changes Apr 8, 2018

View reviewed changes

Upgrade to scalameta v3.7.2

25b8d27

This should fix the CI error on Windows

olafurpg mentioned this pull request Apr 9, 2018

Scalafix does not work on Windows: illegal/unsupported escape sequence #583

Closed

Document chance of deadlock without --metacp-no-par

ef47a2d

olafurpg commented Apr 9, 2018

View reviewed changes

Link to rsc symbol table to credit source of inspiration

5414a5c

olafurpg force-pushed the tpe branch from 8a3f9a0 to 5414a5c Compare April 9, 2018 12:26

olafurpg merged commit 3f070c3 into scalacenter:master Apr 9, 2018

olafurpg deleted the tpe branch April 9, 2018 12:43

olafurpg added this to the v0.6.0-M2 milestone Apr 11, 2018

olafurpg mentioned this pull request May 30, 2018

ExplicitResultTypes crashes on java paths #674

Closed

		import org.langmeta.semanticdb.Symbol
		import scala.meta.internal.semanticdb3.SingletonType.{Tag => x}

Upgrade to scalameta 3.7.0 with default settings. #675

Upgrade to scalameta 3.7.0 with default settings. #675

Conversation

olafurpg commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneDelmore Apr 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneDelmore left a comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg commented Apr 9, 2018

ShaneDelmore Apr 4, 2018 •

edited