-
-
Notifications
You must be signed in to change notification settings - Fork 370
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
Add scala 3 compiler settings test #1484
Conversation
@lefou should I also add also this hint comment to 2.12: @ // interp.preConfigureCompiler(ctx => ctx.setSetting(ctx.settings.YnoImports, true)) // Dotty |
I've no idea. |
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 look good to me. Thank you!
Do you know or have an idea why the test isn't configured for Scala 2.13? Also newer Scala 2.12 version aren't covered?
@lefou there is a note regarding this test not working on 2.13: // not sure why that one doesn't pass in 2.13
// even disabling the noimports and imports settings instead of setting noimports to false
// doesn't seem to reinstate imports
def sv = scala.util.Properties.versionNumberString
// In 2.12.13, I would have expected things like
// interp.configureCompiler(_.settings.Wconf.tryToSet(List("any:wv", "cat=unchecked:ws")))
// to re-instate the expected warning below, to no avail :| I could check this later and verify if we can at least add newer 2.12 versions as part of other MR. |
Yrangepos default was changed between 2.13 versions
0dda475
to
1d1d3d8
Compare
@@ -143,7 +143,7 @@ object BuiltinTests extends TestSuite { | |||
// In 2.12.13, I would have expected things like | |||
// interp.configureCompiler(_.settings.Wconf.tryToSet(List("any:wv", "cat=unchecked:ws"))) | |||
// to re-instate the expected warning below, to no avail :| | |||
if (TestUtils.scala2_12 && sv.stripPrefix("2.12.").toInt <= 12) { | |||
if (TestUtils.scala2_12 && sv.stripPrefix("2.12.").toInt <= 19) { |
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.
Do you know or have an idea why the test isn't configured for Scala 2.13? Also newer Scala 2.12 version aren't covered?
@lefou after covering until 2.12.19
this was successful: mill 'amm.repl[2.12.{13,14,15,16,17,18,19}].test.testOnly' -- 'ammonite.session.BuiltinTests.settings'
.
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.
Great!
bd56404
to
c0b9cd4
Compare
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.
Looks good to me. Thank you!
Enable settings tests for any 2.12 versions. This is a follow up of this conversation #1484 (review) I had previously with @lefou. Tested with `mill -i -w 'amm.repl[{2.12.19,2.13.14,3.4.2}].test.testOnly' -- 'ammonite.session.BuiltinTests.settings'` so all `2.12`, `2.13` and `3` latest versions are passing. Pull request: #1516
Official Compiler Flags documentation do not mention how to set settings for scala 3 compiler.
Maybe adding a comment like this in 2.12?:
Fixes #1192