-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add typeChecks to the library #6175
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
Conversation
Follow up of scala#6135
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
private def typeChecksImpl(code: String)(implicit reflect: Reflection): Expr[Boolean] = { | ||
import reflect._ | ||
typing.typeChecks(code).toExpr | ||
} |
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.
Maybe remove private
, as inlining will create an accessor if it's private.
Can we use top-level definitions to get rid of object TypeChecking
?
Regarding scala.tests
, what about scala.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.
I will try the top level definition and change to scala.test
. typeChecksImpl
must be private or it would be visible under import scala.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.
In the future, we will also support arbitrary code in top-level splices. Which will allow us to implement it like
inline def typeChecks(inline code: String): Boolean = ${
val reflect = implicitly[Reflection]
import reflect._
typing.typeChecks(code).toExpr
}
7449459
to
dc0a51a
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.
LGTM
Follow up of #6135