Migrate to Kotlin DSL #172
Conversation
eyalroth
force-pushed
the
171-kotlin
branch
from
56192f1
to
cc98ce1
Compare
|
You have mixed too many changes (and reasons to change) in a single PR. I don't feel like reviewing so much lines looking for the important changes. I would suggest splitting it, at least in the 3 things I've seen so far: Other than that I left a couple of comments. Feel free to ping me if you want review in smaller PRs. |
| tags = listOf("coverage", "scala", "scoverage") | ||
| } | ||
|
|
||
| apply(plugin = "maven-publish") |
There was a problem hiding this comment.
This can be moved to the plugins block
| } | ||
| } | ||
|
|
||
| configure<PublishingExtension> { |
There was a problem hiding this comment.
And moving maven-publish to the plugins block you won't need to use configure, you'll have the accessor (and you will be able to write it as you did in Groovy)
| sourceCompatibility = JavaVersion.VERSION_1_8 | ||
| targetCompatibility = JavaVersion.VERSION_1_8 |
There was a problem hiding this comment.
Note that these options have zero effect on compilation of non-Java code - that is, plugins like scala or kotlin ignore them completely. The extension documentation explicitly mentions that it is used for compiling Java sources.
That being said, explicitly setting Java compatibility is useful when publishing Gradle module metadata. This area is significantly underdocumented, but from what I see from Gradle sources, if the JVM compatibility attribute value is not explicitly configured for an outgoing configuration, it attempts to deduce its value from the state of the Java compilation tasks (defined in org.gradle.api.plugins.jvm.internal.DefaultJvmPluginServices.DefaultElementsConfigurationBuilder#build). In turn, configuration of the release option on the Java tasks take priority over the targetCompatibility setting, which (if not set on a task directly) is taken from this extension.
With modern Java versions, the recommended way to configure the target Java version is to specify the --release argument to javac, which translates to something like this in Gradle:
tasks.withType<JavaCompile>().configureEach {
options.release.set(8)You may also want to configure Kotlin compilation tasks to target Java 8 explicitly:
tasks.withType<KotlinCompile>().configureEach {
kotlinOptions.jvmTarget = "1.8"
}| compileOnly("org.scoverage:scalac-scoverage-plugin_2.13:1.4.2") | ||
| implementation(group = "commons-io", name = "commons-io", version = "2.6") |
There was a problem hiding this comment.
For consistency, I'd recommend using the same syntax for all dependency declarations.
|
|
||
| mustRunAfter(tasks["test"]) | ||
| } | ||
| tasks["check"].dependsOn(crossScalaVersionTest) |
There was a problem hiding this comment.
| tasks["check"].dependsOn(crossScalaVersionTest) | |
| tasks.check { | |
| dependsOn(crossScalaVersionTest) | |
| } |
This is more idiomatic since it relies on lazy configuration of tasks.
| showStandardStreams = System.getenv("CI") == "true" | ||
| } | ||
|
|
||
| mustRunAfter(tasks["test"]) |
There was a problem hiding this comment.
| mustRunAfter(tasks["test"]) | |
| mustRunAfter(tasks.test) |
Note that if a plugin is applied in the plugins block, its tasks created during the configuration time get typed accessors in the .gradle.kts files, which is the idiomatic way of accessing such tasks.
|
|
||
| val kotlindocJar by tasks.registering(Jar::class) { | ||
| from(tasks.dokkaHtml.get().outputDirectory) | ||
| archiveClassifier.set("kotlindoc") |
There was a problem hiding this comment.
Interesting, is kotlindoc classifier something which is understood by tooling? It's just I never heard about it; from my knowledge, Kotlin API docs are packaged into the regular javadoc classifier JARs.
| @Nested | ||
| var runner: ScoverageRunner? = null | ||
|
|
||
| @InputFiles |
There was a problem hiding this comment.
Note that you usually need to write @get:InputFiles for the annotation to be applied correctly. I think Gradle even fails if it is not like that.
| import org.gradle.api.tasks.PathSensitivity.RELATIVE | ||
| import java.io.File | ||
|
|
||
| abstract class ScoverageAggregate: DefaultTask() { |
There was a problem hiding this comment.
Note that all properties here, including the @Nested one, can be defined as abstract vals, removing the need to call objects methods explicitly.
This is called "managed properties", and is described here. It is considered idiomatic to declare as many properties as possible as managed.
| * Defines a new Test Task which executes normal tests with the instrumented classes. | ||
| * Defines a new Check Task which enforces an overall line coverage requirement. | ||
| */ | ||
| abstract class ScoverageExtension(val project: Project) { |
There was a problem hiding this comment.
Similarly to tasks, you can define all properties here as abstract, removing the need to invoke project directly. You can even make this class into an interface, and configure it in the plugin logic.
| project.plugins.apply(JavaPlugin::class.java) | ||
| project.plugins.apply(ScalaPlugin::class.java) |
There was a problem hiding this comment.
Dependent plugins are conventionally applied inside the plugin, not inside the extension logic.
| deleteReportsOnAggregation.set(false) | ||
| } | ||
|
|
||
| fun check(closure: Closure<*>) { |
There was a problem hiding this comment.
Ideally this should be defined accepting an Action<CheckConfig>. Gradle will automatically generate Groovy closure-accepting method, as long as this class is instantiated through Gradle. Otherwise this API remains hard-to-use from Kotlin DSL.
There was a problem hiding this comment.
Also, consider using NamedDomainObjectContainer for checks, it seems like it is the idiomatic way to construct and configure nested configuration objects (even though in this case, names for checks seem redundant).
Implements #171.