Converted IntTestImage, CustomM2Check CrossVersionTest to Kotlin plug… #4318
Conversation
…ins. Added Config plugin to avoid extra values in Kotlin plugins.
| } | ||
|
|
||
| // I use the tasks.create syntaxt as the crossVersionTest name is already taken by the source set | ||
| val crossVersionTestTask = tasks.create("crossVersionTest", CrossVersionTest::class.java) { |
There was a problem hiding this comment.
Bikeshed: This could be written simpler.
project.task<CrossVersionTest>("crossVersionTest") {
// ect...
}
| description = "Runs the cross-version tests against a subset of selected Gradle versions with 'forking' executer for quick feedback" | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Extra space, if you care.
|
|
||
| if (config.isCiServer) { | ||
| allprojects { | ||
| tasks.withType(Test::class.java) { |
There was a problem hiding this comment.
Reified version of this API exists.
tasks.withType<Test> {
// ect...
}
| @@ -1,3 +1,5 @@ | |||
| import org.gradle.kotlin.dsl.extra | |||
There was a problem hiding this comment.
Import is above the copyright.
|
|
||
| listOf("embedded", "forking").forEach { mode -> | ||
| val taskName = "${mode}CrossVersionTest" | ||
| tasks.create(taskName, CrossVersionTest::class.java) { |
There was a problem hiding this comment.
Same bikeshed as above.
| @@ -0,0 +1,131 @@ | |||
| package org.gradle.plugins | |||
There was a problem hiding this comment.
❓ Is this file meant to be part of this PR? Because I don't see the corresponding Groovy file.
There was a problem hiding this comment.
I'm not found of the plugin id config.
I also think that it mixes up two things:
- build environment values, that are build scoped and don't need to reside on a plugin extension, they could be a simple type in
buildSrc(isCiServerandreleasedVerisions) - on the other hand,
useAllDistributionsis to be set by each project so it would make sense to have it on the extension of a plugin applied to all appropriate projects
| implementationClass = "org.gradle.plugins.CustomM2CheckPlugin" | ||
| } | ||
| "intTestImage" { | ||
| id = "intTestImage" |
There was a problem hiding this comment.
❌ Let's use kebab-case for all plugin ids
|
|
||
| // This is terrible. We should have defined states. | ||
| if (configurations.findByName("partialDistribution") == null) { | ||
| configurations.create("partialDistribution") |
There was a problem hiding this comment.
⭕ Could use configurations.maybeCreate("name") instead.
or configurations { "name"() }
| doLast { | ||
| val m2RepositoryFolder = File("${SystemProperties.getInstance().userHome}/.m2/repository") | ||
| if (m2RepositoryFolder.exists()) { | ||
| project.delete(m2RepositoryFolder) |
There was a problem hiding this comment.
❌ delete(m2repositoryFolder) is enough, no need to spell project. here as we already are in a project.run {} block
|
|
||
| if (config.useAllDistribution) { | ||
| val unpackAllDistribution by tasks.creating(Sync::class) { | ||
| val allZip by project.project("distributions").tasks.getting(AbstractArchiveTask::class) |
There was a problem hiding this comment.
❌ project("distributions")... should be enough
| val selfRunTime by configurations.creating | ||
|
|
||
| afterEvaluate { | ||
| if (project.tasks.findByName("jar") != null) { |
There was a problem hiding this comment.
❌ if(tasks...) should be enough
| } | ||
|
|
||
| ext { | ||
| isCiServer = config.isCiServer() |
There was a problem hiding this comment.
❓ Why keep this extra property now that the value is available from a typed construct?
|
My advice would be to take smaller steps. For example:
|
|
This was one of my initial attempts at migrating to Kotlin. I think the structure is not right and there are other issues as mentioned in the review. Therefore I will close this pull request. |
…ins. Added Config plugin to avoid extra values in Kotlin plugins.
Context
Contributor Checklist
<subproject>/src/integTest) to verify changes from a user perspective<subproject>/src/test) to verify logic./gradlew <changed-subproject>:checkGradle Core Team Checklist