-
Notifications
You must be signed in to change notification settings - Fork 53
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
Create an interface for Kover tasks #338
Comments
Hi, You may add a dependency to a Kover task by its name, for example Names |
Hey @shanshin. Thanks for the info. I disagree with your opinion on this. Making task types public API is the norm. I've never encountered a gradle plugin before this that doesn't have public task classes, regardless how many tasks are created per project. Depending on tasks using Configuring tasks using It seems to me making task types public API has great benefit, no cost, and aligns better with idiomatic gradle usage. |
The described problem is solved by searching for a task by name ( The use of For rare cases, a search by name can be used, which is generally preferable, because in Android projects most often work will be carried out with a specific build variant and the corresponding task.
In fact, there are quite a lot of such tasks, for example, the most frequently used such task is |
In the future, some public classes may be added, however, it is necessary to understand what problem they will solve. At the moment, the only known problem is defining dependencies between tasks, but it is easily solved with the names. For kts build scripts, we will add type-safe accessors for default reports so that users can write |
If I want to reference "koverHtmlReport" in multiple places, I will likely want to define Checking if a task is a kover task based soley on the name will not work in when (task) {
is KotlinCompile, is Jar -> {
...
}
is JavaExec -> {
...
}
.name == KOVER_HTML_REPORT_TASK_NAME -> { // compilation error
...
}
}
(at least for now, until one day KT-21661 is hopefully implemented ) Finding objects by type rather than name is also useful because if the hierarchical structure. If all kover tasks can implement an empty interface The kotlin-gradle-dsl is not available in all places. For example, it is not included by default in the gradle API when writing gradle plugins (and it can be challenging to include the kotlin-gradle-dsl as a dependency for gradle plugins.) So type-safe accessors won't solve this issue as broadly as public classes would. Allowing users to use
I don't think new classes or code needs to be written (though creating a simple hierarchy with interfaces would be excellent). What issues would be introduced by simply removing the Given how small of a change this is that there are identifiable benefits, I'm curious why there is such a need for justification. What costs are there to simply removing the |
It immediately becomes a part of public API which imposes quite a lot of restrictions:
Note that all these problems are solvable and are not arguments against your proposal, but rather an answer to "why it's not public by default" question. To the original report -- thanks for the effort and for going the extra mile in explaining to us why this is a problem for Kover's users! |
This problem is solved by using constants with the task names: e.g.
Unfortunately in this case
At this point, there may be discrepancies in the understanding of the correct hierarchy. After the implementation of #229, new tasks for printing "reports to logs" will appear, and verification will start working in a similar way after #339.
in this case, Android users can find an example of a solution with If the concepts of default reports (always existing tasks with names |
I appreciate the responses. I've never authored a public library before so it is helpful for me to learn about the types of design challenges you confront when considering what to make public API. Thanks both of you for for engaging with me and educating me on this! One thought I had was Opt-in requirements. You could create:
And annotate the class with this. I think this could help in a few ways:
Opt-in requirements seems like a nice catch all solution that could address a number of the concerns you both raised above.
I think this might be ok if it is consistent with other tasks and the norm in the Android ecosystem. I suspect a significant portion at least (if not the majority) of Android Gradle Plugin tasks are both public API (can work in In the end I'll respect the design choices you make. Thank you for clarifying your reasoning. |
I agree that making the tasks // build.gradle
import kotlinx.kover.gradle.plugin.tasks.reports.KoverHtmlTask;
plugins {
id "org.jetbrains.kotlin.jvm" version "1.8.21"
id "org.jetbrains.kotlinx.kover" version "0.7.1"
}
afterEvaluate { // required because of https://github.com/Kotlin/kotlinx-kover/issues/394
tasks.named("koverHtmlReport", KoverHtmlTask).map {
it.reportDir
}
} Another usecase for exposing the task types is to be able to access the input/output properties in a typesafe way. For example, I might want to use the files that a Kover task generates as the input for another task https://docs.gradle.org/8.1.1/userguide/incremental_build.html#sec:task_input_output_side_effects. Because the classes are private then I can't do this. |
You may explicitly specify the report output directory and add a Until the DSL stabilizes, this will be more efficient than getting migration errors when changing the implementation of Kover tasks classes. |
You're right, I shouldn't have said "I can't do this". However, because the classes are private it makes it more difficult to discover this behaviour without digging through the source code or being very savvy with Gradle. It also makes it more difficult to perform actions on a tasks output, e.g. filtering the output val someSpecificKoverFiles = tasks.someKoverTask.map {
it.reportDir.asFileTree.matching { include(...) }.files
} Also, as you have indicated you might refactor the task - so are you sure it's safe to use |
Java reflection is also available, but the question here is not to exclude the technical possibility of accessing the class, but to ensure that users do not randomly start using implementation details for which we do not guarantee backward compatibility.
This is a good use case, the outputs of the task can be listed in the public interface, but the inputs should not be specified when the configuration is carried out through the project extension.
This approach is still under development, but it seems to me that constants should be declared for Kover default tasks names, and for Android tasks it is necessary to create a public function like |
I strongly disagree that Kover has to be so defensive with regards to protecting task properties. Manually modifying the inputs is sometimes necessary. To give one example from a different plugin: to help with integration tests. These properties are already exposed to the public in Groovy scripts, so keeping the tasks as private just makes I disagree that Java reflection to access private code is comparable to Groovy - the former is complicated and the latter is easily accessible, and is even suggested via auto-completion. The approach at the moment is encouraging using random implementation details. Because of using stringly-typed task names, inferring task outputs, and
I understand that dealing with Android is complicated, but thought of having to deal with a plugin that is breaking the norms that all other plugins follow is exhausting. Using Gradle is already difficult as it is. I shouldn't have to leave my IDE in order to figure out how Kover wants me to use Gradle. |
In addition, when writing custom plugins and reacting to other plugins being enabled. Having to guess the task name is not an effective solution. Being able to say "if this task is present in the task graph, finalize it with this task", so that a custom plugin can post process the report generated. With the current behavior I have to duplicate the logic this plugin uses to generate task names, so I can iterate through them to create the necessary dependencies and finalizers. |
- blocks kover and koverReports are merged - added possibility of lazy configuration of Kover extensions - removed the concept of default reports - added the ability to create custom report variants - Created interfaces for Kover tasks Resolves #461 Resolves #410 Resolves #462 Resolves #463 Resolves #338
Thanks! |
To use pattern ``` tasks.withType<KoverReport>().configureEach { dependsOn("otherTask") } ``` it is necessary that all interfaces for Kover tasks inherit the `org.gradle.api.Task` interface Resolves #338
To use pattern ``` tasks.withType<KoverReport>().configureEach { dependsOn("otherTask") } ``` it is necessary that all interfaces for Kover tasks inherit the `org.gradle.api.Task` interface Resolves #338
On a related note, how about exposing In the Kotlin Multiplatform Gradle Plugin extension, there are public kotlin {
macosArm64 {
binaries.configureEach {
// linkTaskProvider is a `TaskProvider<out KotlinNativeLink>` provided by the kotlin plugin
myCustomBuildTask.dependsOn(linkTaskProvider)
linkTaskProvider.configure {
dependsOn(myCustomCheckTask)
}
}
}
} Having public values exposing If you want I can create a separate issue for this, but it falls within the same umbrella of having public APIs for getting references to Kover tasks. |
Could you clarify use case where |
Sure. Use case: I might make my own custom aggregation task per platform for different forms of checks. For example: abstract class MyCustomAndroidChecks: DefaultTask() {
}
val myCustomAndroidChecks by tasks.registering<MyCustomAndroidChecks>()
The purpose of So, I might want to make It could also be the other way around. Like if I wanted the android variant So exposing Which tasks: Currently I use the |
For Kotlin JVM project you already can use For Android projects we can add extensions like |
Thanks, I did not know about the existence of things like For the problem about android variants, what about exposing a |
Comparing with the KMP plugin, you miss a small detail that in KMP there is both the creation of a target and access to its provider in one place. E.g. for example
you take the task in the extension in which you actually create it. When it comes to Kover + AGP, the build variants are specified in AGP, in Kover we cannot know in advance which variants exist and which do not (in some cases they may be created at the Accordingly, even if, for example, we provide such a DSL
then it is necessary to write a custom implementation of As a compromise option, I can suggest adding the
|
Do you know how I might be able to use I am not quite understanding the issue you explain above. Let me propose this: kover {
variants.configureEach {
verifyTaskProvider.configure {
}
}
}
Here, varients is live collection populated lazily. It shouldn't matter that it is created using information from the AGP configuration, or even if it is populated inside of a Is this not possible? Perhaps I am still missing something. Ideally This seems better than relying on a |
But I do see your point about Android variants not being hardcoded. This was my mistake. I see that it would be impossible to hardcode accessors for specific android variants. |
Unfortunately, your plugin will not be able to use this property, because this DSL is available only inside
Technically, this is possible, but there are several difficult points. If we implement a collection of such objects, the case you have given is not very well suited for such an usage: And it doesn't seem to me that such code
would be clearer than
|
You make a great point that kover {
reportVariants.all {
if (variantName in setOf("release", "debug")) {
myTask.configure {
dependsOn(htmlReportProvider)
}
}
}
} kover {
reportVariants {
if (variantName in setOf("release", "debug")) {
myTask.configure {
dependsOn(htmlReportProvider)
}
}
}
} That is because like you say, it is unclear with This would match the kotlin multiplatform plugin DSL. All of these are technically possible: binaries.configureEach { }
binaries.all { }
binaries { } Though normally, we would only use As for val koverHtmlTasks = tasks.withType(KoverHtmlReport::class.java).matching {
it.variantName in setOf("debug", "release")
}
myTask.configure {
dependsOn(koverHtmlTasks)
} Though I believe that if there is any possible alternative to |
Yes, |
Agreed. Using |
Well, is DSL like (in 0.8.0-Beta version)
suitable for you? |
I will need to get the variant for each Kotlin Multiplatform target. I am hopeful this library will eventually support all targets (I know it might still be a while). But if it ever does, I imagine there will be many variants. Referencing them by string will not seem very stable. |
Some ideas:
Is my understanding correct that there would be a variant per target, not per source set? As in, there would never be a "koverCommon" variant, correct? |
It's about current needs. |
Absolutely understandable. I think if kover task interfaces now extend from |
Thanks! I will still add |
Fixed in |
Relates #587 |
I am new to this plugin and I wanted to make certain tasks depend on Kover tasks. However,
KoverHtmlTask
and such areinternal
. This would be fine as long a superclass or interface were made public. But evenAbstractKoverReportTask
is internal. There is no straightforward way to dotask.dependsOn(tasks.withType<KoverHtmlTask>())
.There are a couple of simple solutions. One is that you can make these tasks public and make the constructor internal. Another is that you can create a public interface that they extend from.
The text was updated successfully, but these errors were encountered: