Coursier-small library replaced with Coursier #1443
Conversation
|
Thank you for this contribution! I’m ok with replacing coursier-small with the coursier “interface” module which is a java library that shades coursier and Scala-library. It’s a good idea. It’s not ok however to depend on coursier-cache directly since that will cause binary compatibility issues in the sbt plugin. |
There was a problem hiding this comment.
@poslegm THANK YOU SO MUCH for this valuable contribution!
I left some comments to discuss, but It looks great to me pretty much and it seems to work totally fine in my local environment 👍
| .run() | ||
| val urls = jars.map(_.toPath.toUri.toURL).toArray | ||
| Right(DownloadSuccess(version, urls)) | ||
| } catch { |
There was a problem hiding this comment.
It might be better to use either API that returns Either[CoursierError, Seq[File]] instead of wrapping these with try and catch? Using either would be much more comprehensive rather than pattern matching errors by hand :)
| new RefreshLogger( | ||
| downloadProgressWriter, | ||
| RefreshLogger.defaultDisplay(), | ||
| fallbackMode = true |
There was a problem hiding this comment.
How come it is set to be true?
(I'm not sure how this value affects the behavior or coursier logger... it looks no one refers to this value now... https://github.com/coursier/coursier/blob/354ab7343950e70bb201635167983cacb8eb2c6d/modules/cache/jvm/src/main/scala/coursier/cache/loggers/RefreshLogger.scala#L155-L291 )
What do you think about letting it be a default value for now?
| @@ -11,7 +11,7 @@ Head over to [the user docs][docs] for instructions on how to install scalafmt. | |||
|
|
|||
| - `sbt compile` on a clean machine will fail to compile the `scalafmt-intellij` project. | |||
| - if you plan to develop the intellij plugin, run `downloadIdea` first to fetch the IntelliJ SDK (~600mb). | |||
| - or, run `sbt test` or `sbt core/compile` (specific project). | |||
| - or, run `sbt test` or `sbt coreJVM/compile` (specific project). | |||
| @@ -9,7 +9,7 @@ object Dependencies { | |||
| val scalametaV = "4.2.0" | |||
| val scalatestV = "3.2.0-SNAP10" | |||
| val scalacheckV = "1.13.5" | |||
| val coursier = "1.0.3" | |||
| val coursier = "2.0.0-RC2-5" | |||
There was a problem hiding this comment.
Coursier 2.0.0-RC2-6 is out https://github.com/coursier/coursier/releases/tag/v2.0.0-RC2-6
(Sorry for the delay for reviewing)
|
Thank you very much for review! I am on a vacation without strong internet connection for now. I'll get to work on this in a couple of weeks. |
|
Don't worry about that! Enjoy your vacation 🌴 :) |
| @@ -56,7 +56,7 @@ lazy val dynamic = project | |||
| buildInfoPackage := "org.scalafmt.dynamic", | |||
| buildInfoObject := "BuildInfo", | |||
| libraryDependencies ++= List( | |||
| "com.geirsson" %% "coursier-small" % "1.3.1", | |||
| "io.get-coursier" %% "coursier" % coursier, | |||
There was a problem hiding this comment.
This dependency needs to be replaced with the coursier “interface” module, which is a pure java library with no risk of causing binary incompatibility issues in sbt-Scalafmt. The api of the interface module should be quite similar to the main Scala api.
|
Currently blocked by coursier/interface#57, I will work on it |
|
I opened #1511 porting the usage of coursier-small to the java interface |
|
Closing because of #1511 |
Now scalafmt depends on coursier-small library ― tiny wrapper around coursier. But it is archived and no longer supported.
I see some problems with it:
I tried to replace it with coursier itself. Strict code review is necessary! I tested it locally with sbt plugin and cli client, but we need more complex testing. Any ideas how to do it?
@olafurpg what do you think about this replacement?