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

Move Gradle tasks into task package #2705

Merged

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Oct 11, 2022

part of #2700

depends on

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

@IgnatBeresnev
Copy link
Member

IgnatBeresnev commented Oct 12, 2022

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 IgnatBeresnev added the runner: Gradle plugin An issue/PR related to Dokka's Gradle plugin label Oct 12, 2022
@aSemy
Copy link
Contributor Author

aSemy commented Oct 12, 2022

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
Copy link
Contributor Author

aSemy commented Oct 13, 2022

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.

@IgnatBeresnev
Copy link
Member

👍 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.

@aSemy
Copy link
Contributor Author

aSemy commented Jan 29, 2023

👍 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.

@IgnatBeresnev
Copy link
Member

IgnatBeresnev commented Jan 30, 2023

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
Copy link
Member

IgnatBeresnev commented Feb 21, 2023

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?

@aSemy aSemy force-pushed the feat/move_tasks_into_task_package branch from 8754121 to f231859 Compare February 21, 2023 17:31
Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

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

Thanks!

@IgnatBeresnev IgnatBeresnev merged commit 8c0344e into Kotlin:master Feb 21, 2023
@aSemy aSemy deleted the feat/move_tasks_into_task_package branch February 21, 2023 18:52
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.

None yet

2 participants