Skip to content
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

Fix external documentation link default values #3812

Merged

Conversation

adam-enko
Copy link
Member

@adam-enko adam-enko commented Sep 19, 2024

Prevent the default value of true from overriding the specific values provided from the top level Dokka DSL.

KT-70908

@adam-enko adam-enko added the runner: Gradle plugin An issue/PR related to Dokka's Gradle plugin label Sep 19, 2024
@adam-enko adam-enko added this to the Dokka 2.0.0 milestone Sep 19, 2024
@adam-enko adam-enko marked this pull request as ready for review September 19, 2024 10:56
@@ -218,7 +218,7 @@ constructor(
}

maybeCreate("jdk") {
enabled.convention([email protected])
enabled.set([email protected])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, why does convention not work here?

Copy link
Member Author

@adam-enko adam-enko Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it gets overridden by the configureEach {} above, which sets the convention to true.

externalDocumentationLinks {
configureEach {
enabled.convention(true)
packageListUrl.convention(url.map { it.appendPath("package-list") })
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it skip these known names like if(name !in setOf("jdk", "kotlinStdlib", "androidSdk", "androidX"))?

Usually using actual values instead of conventions in entities that could be configured by users are a code smell.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it looks unusual, but I think it's correct to use set() instead of `convention() here.

The root cause of the non-idiomatic behaviour is that the Dokka DSL is unusual. It has two ways of configuring whether the external links are active - either via DokkaSourceSetSpec#enableAndroidDocumentationLink or via DokkaExternalDocumentationLinkSpec#getEnabled. Why are there two? Which one should take priority? This should be tidied instead.

The actual DSL is less important in DGPv2: the values are determined automatically based on the buildscript environment (e.g. if AGP is applied), so hopefully users don't configure them manually.

In the future I would like to revisit the external documentation links to improve them. I'd like to add a lot more of them, and to dynamically enable/disable them based on the classpath, so no manual configuration is required at all. And to make it easier to register new ones.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create an issue for this to not forget about it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adam-enko adam-enko merged commit 29593f7 into master Oct 1, 2024
14 checks passed
@adam-enko adam-enko deleted the adam/feat/KT-70336/fix-enabling-external-doc-links branch October 1, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runner: Gradle plugin An issue/PR related to Dokka's Gradle plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants