Replace coursier-small with coursier-interface to use the prope… by sswistun-vl · Pull Request #1030 · scalameta/metals

sswistun-vl · 2019-10-30T17:44:42Z

Replaced coursier-small with coursier-interface and added support for supplying custom repositories via environment variable

olafurpg

Looks great! One small question

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

olafurpg · 2019-10-30T17:56:16Z

The test failure looks relevant

X tests.TreeViewLspSuite.libraries 1079ms 
  com.geirsson.coursiersmall.FileException$NotFound: not found: https://repo1.maven.org/maven2/com/google/guava/listenablefuture/9999.0-empty-to-avoid-conflict-with-guava/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava-sources.jar

We should remove all usage of coursier-small, including QuickBuild for the tests

sswistun-vl · 2019-10-31T13:20:07Z

Ok, I've removed all references to coursier-small, but now there are some seemingly unrelated problems, I'm trying to work them out.

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

tests/unit/src/main/scala/bill/Bill.scala

tests/unit/src/main/scala/tests/Library.scala

tests/unit/src/main/scala/tests/QuickBuild.scala

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

olafurpg · 2019-11-02T11:03:12Z

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

+        envRepos
+          .split("""|""")
+          .map(MavenRepository.of)
+          .toList: _*


This is a Java vararg method. toList:_* is fine, we can deal with 2.13 deprecations another time

tests/unit/src/main/scala/tests/Library.scala

tgodzik

Just 2 small things to fix, otherwise LGTM

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

sswistun-vl · 2019-11-12T12:27:08Z

Ok, all previous comments should be resolved, sorry for lack of responses.

tests/unit/src/main/scala/bill/Bill.scala

tgodzik

LGTM

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

tests/unit/src/main/scala/bill/Bill.scala

tests/unit/src/main/scala/tests/Library.scala

tests/unit/src/main/scala/tests/QuickBuild.scala

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

mzarnowski · 2019-11-14T16:56:20Z

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

+    val regex = """([^:]+):([^:]+):([^:]+)""".r
+
+    dep match {
+      case regex(org, name, version) =>


what if the regex doesn't match?

It's just a test, so I wouldn't bother validating this. You can optionally log the unexpected case when the regex doesn't match

Ok, this is not a test. Doing regex without the actual need for them is bad. Let's revert.

Let's keep the dependencies in one like Dependency.of("foo", "bar", "baz") and either keep the regex only in the test scope or drop them altogether

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

tests/unit/src/main/scala/tests/Library.scala

tgodzik

In addition to Marek's review just a tiny additional thing.

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

tgodzik · 2019-11-14T19:45:40Z

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

+    val regex = """([^:]+):([^:]+):([^:]+)""".r
+
+    dep match {
+      case regex(org, name, version) =>


It's just a test, so I wouldn't bother validating this. You can optionally log the unexpected case when the regex doesn't match

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

mzarnowski · 2019-11-15T13:21:48Z

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

+      .dropVendorSuffix(info.getScalaVersion)
+    val pc = Dependency.of(
+      s"org.scalameta",
+      s"mtags_$scala_version",


extract to mtagsVersion

I think this is fine, we don't use it more than once and mtagsVersion would really be less clear here. It's not really mtags version, just mtags + scala version.

It's fine to keep this as is. This is BTW the "mtags artifact name", not "mtags version"

mzarnowski · 2019-11-15T13:22:15Z

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

-      s"mtags_${ScalaVersions.dropVendorSuffix(info.getScalaVersion)}",
+    val scala_version = ScalaVersions
+      .dropVendorSuffix(info.getScalaVersion)
+    val pc = Dependency.of(


can we keep the call in one line like Dependency.of(a, b, c) ?

Scalafmt keeps breaking this line.

It's because it's too long, you would need to extract it to val compilerVersion = mtags.BuildInfo.scalaCompilerVersion. But honestly, I wouldn't bother.

Or this is a different line - got a bit confused

tests/unit/src/main/scala/tests/Library.scala

tgodzik

Let's fix some of the things @marek1840 suggested, but other than that it should be good to merge.

…IntObjectMap.Entry

olafurpg

A few minor comments, otherwise this PR is ready to go!

olafurpg · 2019-11-18T11:06:06Z

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

      scalac: ScalacOptionsItem
  ): URLClassLoader = {
-    val pc = new Dependency(
+    val scala_version = ScalaVersions


nit: in Scala we use camel case instead of snake case

Suggested change

val scala_version = ScalaVersions

val scalaVersion = ScalaVersions

olafurpg · 2019-11-18T11:06:32Z

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala

+      .dropVendorSuffix(info.getScalaVersion)
+    val pc = Dependency.of(
+      s"org.scalameta",
+      s"mtags_$scala_version",


It's fine to keep this as is. This is BTW the "mtags artifact name", not "mtags version"

olafurpg · 2019-11-18T11:08:42Z

tests/unit/src/main/scala/tests/Library.scala

+          Dependency.of("com.lihaoyi", "acyclic_2.12", "0.1.8"),
+          Dependency.of("com.lihaoyi", "scalaparse_2.12", "2.1.0"),
+          Dependency
+            .of("com.typesafe.akka", "akka-cluster_2.12", "2.5.19"),


Pro tip: if you want to avoid the line break here then you can extract the list into a variable in a higher scope like

val dependencies = List( ... )

Scalafmt will always preserve the line break before dot .of(_) so you need to remove that manually if you want to put it back on a single line

mzarnowski · 2019-11-19T12:02:32Z

tests/unit/src/main/scala/tests/Library.scala

+      ("com.typesafe.akka", "akka-cluster_2.12", "2.5.19"),
+      ("com.typesafe.akka", "akka-stream_2.12", "2.5.19"),
+      ("com.typesafe.akka", "akka-testkit_2.12", "2.5.19"),
+      ("io.buoyant", s"linkerd-core_2.12", "1.4.3"),


Suggested change

("io.buoyant", s"linkerd-core_2.12", "1.4.3"),

("io.buoyant", "linkerd-core_2.12", "1.4.3"),

olafurpg

LGTM 👍 Thank you @sswistun-vl ! Nice work, I'm glad to finally get rid of coursier-small ^^

sswistun-vl requested review from mzarnowski, olafurpg and tgodzik October 30, 2019 17:44

olafurpg requested changes Oct 30, 2019

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala Outdated Show resolved Hide resolved

sswistun-vl force-pushed the coursier-interface branch 2 times, most recently from 1945b34 to 8f53fe5 Compare October 31, 2019 12:31

mzarnowski requested changes Oct 31, 2019

View reviewed changes

olafurpg requested changes Nov 2, 2019

View reviewed changes

olafurpg mentioned this pull request Nov 2, 2019

Add basic support for the Pants build tool #935

Merged

sswistun-vl force-pushed the coursier-interface branch from 79ef062 to ed24be1 Compare November 4, 2019 16:36

sswistun-vl requested review from mzarnowski and olafurpg November 4, 2019 16:56

sswistun-vl force-pushed the coursier-interface branch from ed24be1 to a1c7b10 Compare November 5, 2019 11:54

tgodzik changed the title ~~coursier-small changed to coursier-interface~~ Replace coursier-small with coursier-interface to use the proper Coursier API Nov 12, 2019

tgodzik requested changes Nov 12, 2019

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala Outdated Show resolved Hide resolved

metals/src/main/scala/scala/meta/internal/metals/Embedded.scala Outdated Show resolved Hide resolved

sswistun-vl force-pushed the coursier-interface branch from a1c7b10 to 81a304d Compare November 12, 2019 12:26

tgodzik requested changes Nov 12, 2019

View reviewed changes

tests/unit/src/main/scala/bill/Bill.scala Outdated Show resolved Hide resolved

sswistun-vl force-pushed the coursier-interface branch from f2faccb to 83ffe1b Compare November 12, 2019 14:37

tgodzik approved these changes Nov 12, 2019

View reviewed changes

sswistun-vl force-pushed the coursier-interface branch from 83ffe1b to 593a3a7 Compare November 12, 2019 14:43

mzarnowski requested changes Nov 13, 2019

View reviewed changes

sswistun-vl force-pushed the coursier-interface branch 3 times, most recently from 2e94185 to 567b7f7 Compare November 14, 2019 14:13

sswistun-vl requested a review from mzarnowski November 14, 2019 14:57

mzarnowski requested changes Nov 14, 2019

View reviewed changes

tgodzik requested changes Nov 14, 2019

View reviewed changes

sswistun-vl force-pushed the coursier-interface branch from 567b7f7 to d0dafcb Compare November 15, 2019 08:58

sswistun-vl requested review from mzarnowski and tgodzik November 15, 2019 09:16

mzarnowski requested changes Nov 15, 2019

View reviewed changes

tgodzik approved these changes Nov 18, 2019

View reviewed changes

sswistun-vl added 4 commits November 18, 2019 11:48

coursier-small changed to coursier-interface

5fd7ead

removed last coursier-small usages

8bddd4a

netty version change - 4.1 to 4.0 and IntObjectMap.PrimitiveEntry to …

fda9e1d

…IntObjectMap.Entry

removed last coursier-small usages

a2479bf

sswistun-vl force-pushed the coursier-interface branch from 1332dea to b273c63 Compare November 18, 2019 10:54

response to comments

242eeb6

sswistun-vl force-pushed the coursier-interface branch from b273c63 to 242eeb6 Compare November 18, 2019 11:04

olafurpg requested changes Nov 18, 2019

View reviewed changes

mzarnowski approved these changes Nov 18, 2019

View reviewed changes

cleanup

4e069bb

sswistun-vl force-pushed the coursier-interface branch from 10f4053 to 4e069bb Compare November 19, 2019 11:59

sswistun-vl requested review from mzarnowski and olafurpg November 19, 2019 12:00

mzarnowski reviewed Nov 19, 2019

View reviewed changes

cleanup

7e0b4f3

sswistun-vl force-pushed the coursier-interface branch from d373534 to 7e0b4f3 Compare November 19, 2019 14:42

olafurpg approved these changes Nov 20, 2019

View reviewed changes

olafurpg changed the title ~~Replace coursier-small with coursier-interface to use the proper Coursier API~~ Replace coursier-small with coursier-interface to use the prope… Nov 20, 2019

olafurpg merged commit 971c03c into scalameta:master Nov 20, 2019

	val scala_version = ScalaVersions
	val scalaVersion = ScalaVersions

	("io.buoyant", s"linkerd-core_2.12", "1.4.3"),
	("io.buoyant", "linkerd-core_2.12", "1.4.3"),

Conversation

sswistun-vl commented Oct 30, 2019

Uh oh!

olafurpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

olafurpg commented Oct 30, 2019

Uh oh!

sswistun-vl commented Oct 31, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgodzik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sswistun-vl commented Nov 12, 2019

Uh oh!

Uh oh!

tgodzik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tgodzik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgodzik left a comment

Choose a reason for hiding this comment

Uh oh!