-
Notifications
You must be signed in to change notification settings - Fork 409
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
Add example to demonstrate how to configure a custom Dokka plugin #3871
base: master
Are you sure you want to change the base?
Add example to demonstrate how to configure a custom Dokka plugin #3871
Conversation
3430ec8
to
f7d9d09
Compare
- Add custom test tasks to only test a specific example project.
6142c00
to
a93644c
Compare
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.
LGTM
examples/gradle-v2/custom-dokka-plugin-example/demo-library/build.gradle.kts
Show resolved
Hide resolved
@@ -86,6 +97,9 @@ class ExampleProjectsTest { | |||
) { | |||
val exampleProjectName = ExampleProject.of(project.projectDir) | |||
|
|||
val isEnabled: Boolean = | |||
project.projectDir.name == System.getProperty("exampleProjectFilter") |
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 if you run these via :dokka-integration-tests:gradle:testExampleProjects
they are now all going to be skipped. Do the CI builds now use per example project runs?
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.
Would it be better to use something like:
val isEnabled: Boolean = System.getProperty("exampleProjectFilter").let {
if (it == null) return@let true
return@let it == project.projectDir.name
}
?
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.
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.
Judging by the log the per-project tasks get pulled in by the integrationTest
task though I wonder why this happens and how easily this is broken
@@ -215,5 +219,28 @@ testing { | |||
systemProperty("junit.jupiter.execution.parallel.mode.default", "CONCURRENT") | |||
} | |||
} | |||
|
|||
// Register specific Gradle test runs for each example so the examples can be tested individually. |
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.
nit: we discussed this in the previous PR, but I still feel that it would be more conventional to have a generated testGen
source directory (under git and with an idempotency check) than a per test project Gradle task
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.
One of the benefits is that you could then run an arbitrary subset of such tests using the standard Test
task's --tests
filter
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.
Yes, those would be good benefits. I'm just struggling to think about how to generate tests dynamically though, because I usually find code generation inconvenient and prone to mistakes.
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.
If you decide to look further into the direction of generating test cases, I think the setup is usually something like:
src/MyTestsInfrastructure.kt
abstract class TestsInfrastructure {
// hand craft some eloquent testing infrastructure with an API that easy to generate in 1 LOC
fun runTest(testDataPath: String) { ... }
}
src/tests-gen/Tests.kt
// This class is generated
class Tests : TestsInfrastructure {
@Test
fun test_basicExample() = runTest("path/to/basicExample")
}
|
||
return """ | ||
{ | ||
"annotatedWith": $annotatedWithJson |
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 am a bit surprised Dokka doesn't support the standard Java serialization for this configuration
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.
nit: wdyt about suggesting:
buildscript {
dependencies {
classpath("org.jetbrains.kotlinx:kotlinx-serialization-json:1.6.3")
}
}
and serializing using:
return Json.Default.encodeToString(
JsonObject.serializer(),
JsonObject(
mapOf("annotatedWith" to JsonArray(annotatedWith.get().map { JsonPrimitive(it) }))
)
)
?
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.
If I were doing this in my own project, I would use Kotlinx Serialization. But because this is a basic example, I want to show the most simple configuration possible, even if manually creating Json is ugly and invites mistakes.
In the future we want to improve the experience of configuring custom Dokka plugins (#3869, #3925), and provide a better wrapper.
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.
Ok. I wonder if it would be simpler to just provide a File
as a transport between Gradle -> Dokka Plugin and then let the user choose an arbitrary serialization format
|
||
@get:Input | ||
@get:Optional | ||
abstract val annotatedWith: ListProperty<String> |
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.
nit: IIUC this sample primarily shows how you parametrize a dokkaPlugin
, but I am not sure whether the ListProperty
makes sense for the use case of suppressing some documentation; would you want to suppress documentation using multiple different annotations? I feel it might be clearer to use a Property<String>
with a single FQN and then the JSON serialization can also be simplified
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.
Good idea, I'll implement this.
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.
Could you please go through my comments. I am primarily concerned whether we might be accidentally skipping the example IT in CI runs
Add an example to demonstrate how to configure a custom Dokka plugin using DGPv2.
DGPv2 has much more strict requirements for configuring a Dokka plugin, to ensure that inputs/outputs for a Gradle task are properly configured. However, this does make the configuration harder to write. This is something we could look at improving in the future, but for now, here is an example to demo how it can be done (albeit in a verbose and unintuitive/Gradle way).
#3869
To verify, check the expected docs.
fun main()
is not visible: