-
Notifications
You must be signed in to change notification settings - Fork 90
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
enable DottyJS, drop Scala.js 0.6.x #248
Conversation
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.
Thank you for this contribution!
I'm OK with dropping 0.6.x support. Can we update the 0.6.x column on the website to list the 0.7.16 as the last version supporting 0.6.x? https://scalameta.org/munit/docs/getting-started.html
A few nitpick comments regarding parentheses, otherwise looks good to me!
@@ -1,11 +1,11 @@ | |||
package munit.internal.junitinterface | |||
|
|||
trait Settings { | |||
def trimStackTraces(): 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.
Can we add the parentheses back here? These interfaces are meant to be mirrors of their equivalent Java interfaces https://github.com/scalameta/munit/blob/7ea3075ae51ca73c7fa84373aa9dd82cf9a6ccd7/junit-interface/src/main/java/munit/internal/junitinterface/Tag.java
I prefer to keep the two synchronized as much as possible, these are internal APIs so it shouldn't matter much whichever we use.
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 can add them back, but that makes stuff in some other places more complicated. There are some classes that attempt to use val
to override those def
s (which doesn't work anymore in Dotty, so they have to be renamed). Those constructors are subsequently called with named parameters, so their call sites have to be renamed too.
I'm happy to go ahead either way, though.
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 see. That makes sense, I'm glad Scala 3 is more strict about the differences between method()
and method
without parentheses. It's still quite a large breaking change, I always thought it was elegant that val
could override parameterless defs.
Anyways, I'm fine dropping the parentheses in this case 👍
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.
No need to change the parentheses, just the Scala.js 0.6 support mention in the docs.
@@ -1,11 +1,11 @@ | |||
package munit.internal.junitinterface | |||
|
|||
trait Settings { | |||
def trimStackTraces(): 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.
I see. That makes sense, I'm glad Scala 3 is more strict about the differences between method()
and method
without parentheses. It's still quite a large breaking change, I always thought it was elegant that val
could override parameterless defs.
Anyways, I'm fine dropping the parentheses in this case 👍
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.
LGTM 👍 Thank you!
Kickstarted 0.7.17 https://github.com/scalameta/munit/actions/runs/354125829 |
Drop Scala.js 0.6.x and enable DottyJS for 3.0.0-M1.
I haven't enabled DottyJS for 0.27 because tests are somehow failing there.
Should help fix #199. Obsoletes #246.