-
Notifications
You must be signed in to change notification settings - Fork 3
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
Support pure JavaModule
s
#155
Conversation
Looks like there is no need for being a `ScalaModule` at all. There is some sanity check, but it looks like not really needed. Fix #154
Can you add a test that checks some java code? |
case _ => | ||
// FIXME: ask Mill, but `mill.main.BuildInfo` isn't available in older versions | ||
T.task("2.13") |
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 _ => | |
// FIXME: ask Mill, but `mill.main.BuildInfo` isn't available in older versions | |
T.task("2.13") | |
case _ => T.task("3") |
I would set as "3" so it will continue to work for at least two decades. We don't even need to change it to mill.main.BuildInfo
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.
Actually, it can be null
. Let me push a commit on top
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 could also remove the whole sanity check or make it optional by using an Option[String]
for the Scala version.
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 used String or null
since Option
can't be used directly with the Java API, and I didn't want to use java's Optional
.
I pushed a commit on top
167408d
to
e8b380f
Compare
Very cool that MiMa can be used to test Java projects! It's a very useful tool, maybe this can be another differentiator for Java folks to move to Mill! :) |
Yeah, I think we could even drop the need to be a |
|
||
val errorOrNull: String = | ||
mimaWorker().reportBinaryIssues( | ||
scalaBinVersionOrNullTask().getOrElse(null), |
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 Option
has a .orNull
method
scalaBinVersionOrNullTask().getOrElse(null), | |
scalaBinVersionOrNullTask().orNull, |
Huh, itest for Mill 0.11.0 fails with:
But I fail to see where we still require ScalaModule |
Oh, there is an issue with the |
Instead of extending `ScalaModule` it should use the `bindDependency` task provided by `CoursierSupport`. Otherwise, it can be problematic in downstream modules.
This is good for a next review now. Once merged, a new release would be nice. |
Looks like there is no need for being a
ScalaModule
at all.There is some sanity check, but it looks like not really needed.
Fix #154
I also fixed an unrelated issue with
ExtraCoursierSupport
, which was inheritingScalaModule
. Instead of extendingScalaModule
it should use thebindDependency
task provided for exactly that use case byCoursierSupport
. Without this fix, extendingExtraCoursierSupport
can be problematic in downstream modules.