Skip to content

Conversation

tgodzik
Copy link
Collaborator

@tgodzik tgodzik commented Jul 21, 2025

No description provided.

@tgodzik tgodzik force-pushed the update-evaluator branch 4 times, most recently from c1b4f8f to 8a31772 Compare July 22, 2025 08:01
@tgodzik tgodzik marked this pull request as ready for review July 22, 2025 08:25
@adpi2
Copy link
Member

adpi2 commented Jul 22, 2025

@tgodzik thank you for pushing this forward. I'll take a look at it soon.

@adpi2 adpi2 self-requested a review July 22, 2025 08:43
@tgodzik tgodzik force-pushed the update-evaluator branch from 8a31772 to 25a5feb Compare July 22, 2025 10:50
@tgodzik
Copy link
Collaborator Author

tgodzik commented Jul 24, 2025

@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.

Copy link
Member

@adpi2 adpi2 left a 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?

Comment on lines 113 to 122
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
Copy link
Member

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.

Copy link
Collaborator Author

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

@tgodzik
Copy link
Collaborator Author

tgodzik commented Jul 25, 2025

Also, could you add a test to make sure that we can invoke the Scala 3.7 expression compiler?

I added a test that invokes 3.7.2-RC2, or did you mean something else?

@tgodzik tgodzik requested a review from adpi2 July 25, 2025 11:23
@WojciechMazur
Copy link

Hi @adpi2 , when you get a moment, could you please take a look at this PR agin?
It might be blocking the upcoming Scala compiler release. Would appreciate your input to help move it forward. Thanks in advance!

@tgodzik
Copy link
Collaborator Author

tgodzik commented Jul 29, 2025

Hi @adpi2 , when you get a moment, could you please take a look at this PR agin? It might be blocking the upcoming Scala compiler release. Would appreciate your input to help move it forward. Thanks in advance!

I went ahead with a new release, so we should be good for now

Copy link
Member

@adpi2 adpi2 left a 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!

@tgodzik tgodzik force-pushed the update-evaluator branch from 875c277 to ca0672f Compare July 31, 2025 11:53
@tgodzik tgodzik merged commit 858e0bc into scalacenter:main Jul 31, 2025
9 checks passed
@tgodzik tgodzik deleted the update-evaluator branch July 31, 2025 13:08
tgodzik added a commit that referenced this pull request Sep 1, 2025
improvement: Use expression evaluator from the compiler for 3.7.0+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants