- Notifications
You must be signed in to change notification settings - Fork 25
improvement: Use expression evaluator from the compiler for 3.7.0+ #884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c1b4f8f
to 8a31772
Compare @tgodzik thank you for pushing this forward. I'll take a look at it soon. |
8a31772
to 25a5feb
Compare @adpi2 would you be able to take a look? We would want to have it ready in time for the next Scala release. It's totally ok if you don't have time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, could you add a test to make sure that we can invoke the Scala 3.7 expression compiler?
...gin/src/main/scala/ch/epfl/scala/debugadapter/sbtplugin/internal/SbtDebugToolsResolver.scala Outdated Show resolved Hide resolved
val expressionCompilerArtifact = | ||
s"${BuildInfo.expressionCompilerName}_${scalaVersion.value}" | ||
val (expressionCompilerOrg, expressionCompilerArtifact, expressionCompilerVersion) = | ||
if (scalaVersion.isScala3 && scalaVersion.minor >= 7) { | ||
("org.scala-lang", s"scala3-compiler_3", scalaVersion.value) | ||
} else { | ||
(BuildInfo.organization, s"${BuildInfo.expressionCompilerName}_${scalaVersion.value}", BuildInfo.version) | ||
} | ||
val expressionCompilerDep = Dependency( | ||
coursier.Module(Organization(BuildInfo.organization), ModuleName(expressionCompilerArtifact)), | ||
BuildInfo.version | ||
coursier.Module(Organization(expressionCompilerOrg), ModuleName(expressionCompilerArtifact)), | ||
expressionCompilerVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I think the code would be simpler to understand if ScalaInstance.expressionCompilerJar
becomes a Option[Library]
.
fetchScala3
would become a if else
expression. If 3.7, we resolve the compiler and there is no expressionCompilerJar
, else we resolve the expressionCompilerArtifact
as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it, tough we still fetch the compiler jars, so I had to leave the code more or less the same. Otherwise we would just be duplicating things
...s/core/src/main/scala/ch/epfl/scala/debugadapter/internal/evaluator/ExpressionCompiler.scala Outdated Show resolved Hide resolved
modules/core/src/main/scala/ch/epfl/scala/debugadapter/internal/DebugTools.scala Show resolved Hide resolved
I added a test that invokes 3.7.2-RC2, or did you mean something else? |
Hi @adpi2 , when you get a moment, could you please take a look at this PR agin? |
I went ahead with a new release, so we should be good for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay.
That looks great!
875c277
to ca0672f
Compare improvement: Use expression evaluator from the compiler for 3.7.0+
No description provided.