Clean up - #27
Conversation
| val fields = decodeTaskInput(expectedFieldCount = 4).iterator() | ||
| return DeclaredDependency( | ||
| group = group, | ||
| artifact = artifact, | ||
| configuredVersion = configuredVersion.takeIf(String::isNotEmpty), | ||
| configurationName = configurationName, | ||
| group = fields.next(), | ||
| artifact = fields.next(), | ||
| configuredVersion = fields.next().takeIf(String::isNotEmpty), | ||
| configurationName = fields.next(), |
There was a problem hiding this comment.
Should fix — decoding now depends on named-argument evaluation order.
The previous val (group, artifact, configuredVersion, configurationName) = decodeTaskInput(expectedFieldCount = 4) bound fields positionally: order-independent and arity-checked by the compiler. The iterator form is correct only because Kotlin evaluates call arguments in source order, so reordering these named arguments — a normally safe, IDE-suggested edit — would silently misassign configuredVersion and configurationName with no compile error.
toResolvedVersion() at line 305 still destructures, so the two sibling decoders now read differently for no stated reason. If the rewrite was needed to satisfy Detekt's DestructuringDeclarationWithTooManyEntries (default maxDestructuringEntries: 3), which now applies to buildSrc for the first time, that is worth a comment.
Smallest safe correction: keep the decoded List and index it (fields[0]…fields[3]), which restores positional binding without a 4-entry destructuring.
| This repository has no `.gitmodules`, so it has no submodule contents or | ||
| `config`-distributed files to exclude. Treat every tracked path as project-owned, | ||
| including `buildSrc/`, `AGENTS.md`, `CLAUDE.md`, `gradle.properties`, | ||
| `.github/copilot-instructions.md`, and `.idea/`. Its `config/` directory contains | ||
| project-owned Detekt configuration. |
There was a problem hiding this comment.
Should fix — .idea/ is not a tracked path in this repository.
The sentence says "Treat every tracked path as project-owned, including … and .idea/", but .idea/ is ignored here (.gitignore:3) and git ls-files .idea returns nothing. The other four paths named are tracked; only this one is not.
This page is the authority agents consult before editing files, so a path that does not exist in the repo weakens it. Either drop .idea/ from the list, or say explicitly that it is ignored here and therefore out of scope.
| fun `compare numeric version segments numerically`() { | ||
| assertTrue(dependencyVersionComparator.compare("1.10", "1.9") > 0) |
There was a problem hiding this comment.
Should fix — one assertion for a comparator that was widened to internal in order to test it.
dependencyVersionComparator went from private to internal for this test, but only 1.10 > 1.9 is covered. Replaying the comparator's algorithm (Regex("""\d+|\D+""") segments, then firstSegments.size.compareTo(secondSegments.size) as the tie-break) shows two unverified behaviours:
| comparison | result | selected by maxWith |
|---|---|---|
2.0.0-alpha.5 vs 2.0.0 |
+1 |
2.0.0-alpha.5 |
1.0.0-RC1 vs 1.0.0 |
+1 |
1.0.0-RC1 |
1.09 vs 1.9 |
0 |
either |
A prerelease outranks its own release because it simply has more segments. selectDependencies picks resolvedVersions[id]?.maxWithOrNull(dependencyVersionComparator), so a module resolving to both an alpha and its release would be documented at the alpha — and the build already carries dev.detekt:…:2.0.0-alpha.5.
The comparator body is pre-existing and unchanged by this PR; impact is confined to the generated pom.xml and dependencies.md, never the published artifact. Since the PR is what makes the comparator testable, this is the natural place to add prerelease and leading-zero cases (and to decide whether the current ordering is intended).
| tasks.register<Jar>("javadocJar") { | ||
| description = "Packages Kotlin API documentation generated by Dokka." | ||
| archiveClassifier.set("javadoc") |
There was a problem hiding this comment.
Nit — the replacement task loses the build group.
The ordering trick works: Gradle's withJavadocJar() guards with if (!tasks.getNames().contains(jarTaskName)), so this task is reused and wired into javadocElements. Verified locally — ./gradlew :gradle-plugin:javadocJar produces a 640 KB jar with 120 Dokka HTML entries.
One detail is dropped, though: the task Gradle would have created also sets group = BasePlugin.BUILD_GROUP. Because this one only sets description, ./gradlew :gradle-plugin:tasks --all now lists javadocJar under Other tasks, while sourcesJar stays under Build tasks. Adding group = LifecycleBasePlugin.BUILD_GROUP restores the parity.
| dokka { | ||
| dokkaPublications.html { | ||
| moduleName.set("embed-code-gradle-plugin") | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit — a shared convention hardcodes one module's name.
dokka-configuration lives in buildSrc alongside jvm-module, so it reads as reusable, but moduleName is pinned to embed-code-gradle-plugin. A second module applying it would publish its API documentation under the wrong name, silently.
Either derive it (base.archivesName, which gradle-plugin/build.gradle.kts:121 already sets to the same value), or leave the convention to apply and configure the plugin and move the moduleName override into gradle-plugin/build.gradle.kts where the name belongs. Not blocking while there is exactly one consumer.
| @TempDir | ||
| lateinit var projectDirectory: Path |
There was a problem hiding this comment.
Nit — the field name does not describe what the directory holds.
This is not the Gradle project's directory: every test builds its project with ProjectBuilder.builder().build(), which allocates its own temporary directory. This @TempDir is only the parent of the fixture Maven repository created by createMavenModule, and only one test reads it. repositoryRoot (or similar) would say so.
Oleg-Melnik
left a comment
Oleg-Melnik
left a comment
There was a problem hiding this comment.
@Vladyslav-Kuksiuk LGTM with comments to address.
2 participants
This PR: