twirl icon indicating copy to clipboard operation
twirl copied to clipboard

Prepend "using" at the callsite when definition site has implicit parameter

Open ajafri2001 opened this issue 11 months ago • 4 comments

Fixes issue-894

ajafri2001 avatar May 11 '25 23:05 ajafri2001

I should add tests and only prepend "using" for scala3+ :sweat_smile:.

I'm not sure if this pr is the correct way to solve the issue tbh, if you have something particular in mind I'm willing to work on it.

ajafri2001 avatar May 12 '25 00:05 ajafri2001

@ajafri2001 Hi. I tried out this pull request, and as far as I can tell, it seems to be working well. I left a few minor comments—please take a look. Also, I think it would be good to add a test case to CompilerSpec. You can find various test files in compiler/src/test/resources/, so I recommend referring to those as examples.

tototoshi avatar Jun 17 '25 00:06 tototoshi

@tototoshi Hi, I'm unable to get the CompilerSpec Tests to compile with scala3 instead of scala2, I have added the tests but they now fail.

Edit - Also the parser does not recognize using instead of implicit. I'm willing to work on that too, if you can point me in the right direction

ajafri2001 avatar Jun 17 '25 18:06 ajafri2001

On my end, the tests seem to pass with Scala 3, but not with Scala 2

Also the parser does not recognize using instead of implicit.

I had the same thought as well. I haven’t looked into it deeply yet, but the TwirlParser probably only looks at parentheses and doesn’t check for implicit or using keywords. So I don’t think any changes are needed there.

As for the TwirlCompiler, I think your current fix could be extended slightly to support this case too.

tototoshi avatar Jun 23 '25 12:06 tototoshi

@tototoshi

the TwirlParser probably only looks at parentheses and doesn’t check for implicit or using keywords. So I don’t think any changes are needed there. As for the TwirlCompiler, I think your current fix could be extended slightly to support this case too.

Yes, you were right this worked 🙇‍♂️, twirl is now able to compile the syntax @(....)(using some_name: some_type).

The test fails now because sbt test by default runs with scala2, and I'm not sure how to test with scala3

ajafri2001 avatar Jun 23 '25 23:06 ajafri2001

You can do it like this:

sbt
> ++3.3.5
> test

or

sbt ++3.3.5 test

tototoshi avatar Jun 28 '25 11:06 tototoshi

The reason the tests are failing on Scala 2 is not due to the callsite, but because there's an error at the definition site. So I believe it's unrelated to the changes in this PR. Also, it would be quite unnatural for Scala 2 users to write using in their templates. Given that, I think it's reasonable for Twirl to simply not support using when running under Scala 2.

tototoshi avatar Jun 28 '25 11:06 tototoshi

Ahh, the tests fail with scala2 but pass with scala3 because, as you mentioned, using doesn't exist in scala2. this pr enables the use of using at the definition site as well as the call site for scala3, solving both issues issue-894 and issue-839, which are specifically for scala3 and are both addressed by this pr.

However, the test i've added namely using.scala.html, is only ever supposed to run for scala3 and shouldn't be run with scala2 at all.

How should i deal with this situation and make the test run ONLY with scala3 (since it's a scala3 specific feature)? I'm really grateful for your help so far :sweat_smile:

ajafri2001 avatar Jun 28 '25 12:06 ajafri2001

If you want a test to run only on Scala 3, you can place it under src/test/scala-3. However, that approach requires splitting tests by class, which might be a bit cumbersome.

Alternatively, a simpler way is to check the Scala version at runtime and decide whether to run the test. With ScalaTest, you can use assume like this:

assume(BuildInfo.scalaVersion.startsWith("3."), "This test is only for Scala 3")

tototoshi avatar Jun 28 '25 13:06 tototoshi

Thanks the tests pass now

ajafri2001 avatar Jun 28 '25 14:06 ajafri2001

@Mergifyio backport 2.0.x 1.6.x

mkurz avatar Jun 30 '25 11:06 mkurz

Released:

  • https://github.com/playframework/twirl/releases/tag/2.0.9
  • https://github.com/playframework/twirl/releases/tag/1.6.10

mkurz avatar Jun 30 '25 13:06 mkurz

@tototoshi @mkurz Omg thanks for the all the help and the shoutout!

ajafri2001 avatar Jun 30 '25 13:06 ajafri2001