-
Notifications
You must be signed in to change notification settings - Fork 394
Twirl support in metals #7751
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
base: main
Are you sure you want to change the base?
Twirl support in metals #7751
Conversation
Add twirl support
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.
Thanks for the great work! There is some improvements here to do, but this will already be a great start if anyone wants to add further features.
| // A bit suspicious o_O | ||
| if (source.toNIO.toString.isTwirlTemplate) { | ||
| return inverseSources( | ||
| source.parent.resolveSibling(_ => "scala").resolve("foo.scala") |
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.
This should be removed
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.
Done! This is removed, although sbt-twirl doesn't provide a build-target by itself unlike play project, so people who want to use standalone twirl would get No Build Target Found. Although I'm not really sure how this all works 😅 so take this with a grain of salt 🧂
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.
We can fix that later then.
| path, | ||
| diagnostics.getOrElse(path, new ju.LinkedList[DiagnosticWithOrigin]()), | ||
| ) | ||
| if (!path.isTwirlTemplate) { |
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.
I think we do want to publish those diagnotics from the build server. We should instead change the didChange method in Trees to:
case Some(parsed) if !path.isTwirlTemplate =>
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.
Oh good point.
| } | ||
| } | ||
|
|
||
| def isPlayProject(implicit file: VirtualFile): Boolean = |
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.
AS an improvement, we could check in sbt project for play specific libraries. But this seems good enough 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.
That does make sense.
|
|
||
| println("This is edits -> " + edits) | ||
|
|
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.
| println("This is edits -> " + edits) |
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.
lol how did this get through 😅
| if (locations.asScala.forall(loc => originalUri.equals(loc.getUri()))) | ||
| adjust.adjustLocations(locations) |
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.
| if (locations.asScala.forall(loc => originalUri.equals(loc.getUri()))) | |
| adjust.adjustLocations(locations) | |
| val adjustable = locations.asScala.filter(loc => originalUri.toString == (loc.getUri())) | |
| adjust.adjustLocations(adjustable) |
two issue here, we:
- equals didn't work because originalUri is an Uri, so different classes will never equal
- there might be a rare scenarion with go to definition that it will return multiple locations (probably not in twirl, but other places)
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.
Whoops
| private def resolveVersion(scalaVersion: String): String = { | ||
| val base = scalaVersion.split('-')(0) | ||
| base match { | ||
| case v if v.startsWith("2") => v.split('.').take(2).mkString(".") |
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.
| case v if v.startsWith("2") => v.split('.').take(2).mkString(".") | |
| case v if v.startsWith("2") =>ScalaVersions.scalaBinaryVersionFromFullVersion(v) |
also, do you know why for Scala 3 we have a full version and not just "3" ?
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.
To be honest, I had this function for resolving file paths when we used the compiled .template.scala files from target/scala-XX/twirl/main/example.template.scala. I noticed that Scala 2 versions usually took the form target/scala-2.13/ or target/scala-2.12/, while in Scala 3, they took the form target/scala-3.7.2/, etc. This means that, Scala 2 versions include only the major and minor version numbers, whereas Scala 3 versions also include the patch version at the end (e.g., 3.7.2 instead of just 3.7).
After we started compiling it in memory, it served no real purpose.
why for Scala 3 we have a full version and not just "3"
TwirlCompiler.scala checks for scala versions like this, .startsWith("3.") instead of .startsWith("3"). So if we just do scalaVersion = Some("3"), it might get interpreted as compiling for a scala2 version instead 🫢.
One way we could do is remove the method resolveVersion entirely and just do Some(scalaVersion) instead of Some(resolveVersion(scalaVersion)). What do you think? 🤔
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.
If it works I would opt for removing the method.
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.
It worked!, I will push it along maybe some more commits so that the CI won't run for all of the commits.
PS - Is there a way on github to stop CI from running automatically? Because sometimes it's just a commit that changes comments etc, and I think the CI should be run automatically only when the full test suites are needed, or maybe when it's not a draft pr atleast 🤔.
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.
You can add a tag to the commit like [skip ci], though that might only work in the PRs, not sure
| * @return An array of tuples representing (originalIndex, generatedIndex) pairs | ||
| */ | ||
| private def getMatrix(compiledTwirl: String): Array[(Int, Int)] = { | ||
| val pattern = """(\d+)->(\d+)""".r |
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.
You can extract that patter outside so that it's not recompiled every 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.
Done!
| _ <- assertCompletionEdit( | ||
| "File@@", | ||
| """ | ||
| |@import java.io.File |
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.
There seems to be small issue that if a second symbol such as Paths is imported then no newline is inserted.
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.
was able to reproduce it, will fix it and add it to the twirl-import test.
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.
Hmm, the fix is kinda weird for me 😅 , and I couldn't find something satisfactory yet, for now instead of prepending @, I basically prepend \n@ , this provides autoimports that actually work, but there is an extra new line at the start 😢 .
Also, I could've swore the imports were working correctly on sbt-twirl instead of an the webjar play project. Have to investigate more 🫨 .
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.
this might be due to the reverse mapping. We might be losing some newlines because of that. MAybe it's fine to add \n@ for now 🤔
| res <- definitionsAt( | ||
| "src/main/twirl/example.scala.html", | ||
| """|@(name: String) | ||
| |<h1>Hello @name.toI@@nt</h1> |
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.
We should also add a check for "name", can just be the next step like:
res2...
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.
Done!
|
@ajafri2001 will you have time to finish this? |
oh yes I forgot, I'll wrap it up by tomorrow. |
|
Am in last stretch of exam season, HAVE NOT ABANDONED! |
LSP support for Play Framework's Twirl Files.
This work was done as part of Google Summer of Code 2025. The last commit as of 1st September 2025 is this.
Hover
Completion
Goto-Definition
Auto-Import
Rename