dokka icon indicating copy to clipboard operation
dokka copied to clipboard

Move Gradle tasks into task package

Open aSemy opened this issue 1 year ago • 3 comments

part of #2700

depends on #2701 and #2702

This helps organise the package to make it a little more understandable. All the Gradle tasks are stored in one package.

aSemy avatar Oct 11 '22 20:10 aSemy

While I agree that it'd be nice to change structure here, I think we're going to decline this change since package names are part of public API already, it's used to configure Dokka:

import org.jetbrains.dokka.gradle.DokkaTask

val dokkaHtml by getting(DokkaTask::class) {
    outputDirectory.set(buildDir.resolve("dokka"))
    ...
}

Changing the package as proposed might break builds upon update. It's not that difficult to correct imports, but many projects rely on github bots for updating versions, so it's a bit more painful than it might appear at first

We could take this step, but we plan to integrate Dokka into Kotlin Gradle Plugin at some point (so it's built in, and not a separate dependency), so these tasks might or might not be moved there or some place else, we're not sure. I'd like to avoid introducing too many compatibility problems before then.

We can get back to it as soon as we start integrating Dokka into KGP, if it's still relevant

IgnatBeresnev avatar Oct 12 '22 20:10 IgnatBeresnev

Understood, this would be a breaking change.

As a compromise, what about moving the files so they are visually organised, but retaining the package name so the code stays the same? Although I'm not convinced of this myself, I think it would lead to more confusion and perhaps problems.

aSemy avatar Oct 12 '22 21:10 aSemy

Alternatively, use a more convention method of leaving the classes in the current package and marking them with @Deprecated and @ReplaceWith. Because the tasks are abstract classes, they could even extend the moved classes.

I'll update the PR as a demo (because it's a small amount of work) - but I would understand if you still want to decline this PR.

aSemy avatar Oct 13 '22 11:10 aSemy

:+1: I also thought about using typealiases to mitigate the breaking changes.

I would still postpone this PR until we have some other breaking changes that require the users to change their configuration, like #2752 or maybe some other Gradle-related refactorings. So that there's a single breaking release, instead of multiple ones in a row

We'll discuss our plans for the near future and try to find a place for it.

IgnatBeresnev avatar Jan 29 '23 13:01 IgnatBeresnev

👍 I also thought about using typealiases to mitigate the breaking changes.

I would still postpone this PR until we have some other breaking changes that require the users to change their configuration, like #2752 or maybe some other Gradle-related refactorings. So that there's a single breaking release, instead of multiple ones in a row

We'll discuss our plans for the near future and try to find a place for it.

Whenever you think is best 👍 But the breaking change would be the PR that removes typealiases. I think it's best to share the deprecations as soon as possible, because then it gives more opportunity for the deprecation warning to be seen and acted on.

aSemy avatar Jan 29 '23 17:01 aSemy

But the breaking change would be the PR that removes typealiases.

Sorry, "breaking change" was not the best term to use. I meant to describe the situation when a user has to take action. If we merge this and they update Dokka, they'll start seeing deprecation warnings (or maybe even build failures due to failOnWarning?), so they'll have to go and investigate. Given that some people update dependencies via automatic bots, it can take more time/effort to update Dokka than expected.

For me it makes sense to group such changes in one release, and there's a very big chance we'll have something similar that requires user action in 1.8.20. We'll know for sure after we have an internal planning after the closest release, hopefully I won't forget to give an update in this PR.

IgnatBeresnev avatar Jan 30 '23 13:01 IgnatBeresnev

The plans for #2752 or any other "breaking" changes are still unclear, but the changes from this PR would improve maintainability of the Gradle plugin significantly, so I really want to merge it one way or another

Proposing to just move the task classes into the physical directory org.jetbrains.dokka.gradle.tasks, but leave the package statement as package org.jetbrains.dokka.gradle, so that it doesn't break source compatibility.

The warnings will have to be suppressed with a comment though, which can be a link to this message/PR

Also, can you please rebase onto latest master?

IgnatBeresnev avatar Feb 21 '23 16:02 IgnatBeresnev