-
Notifications
You must be signed in to change notification settings - Fork 415
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
Move Gradle tasks into task package #2705
Conversation
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 |
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. |
Alternatively, use a more convention method of leaving the classes in the current package and marking them with 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. |
👍 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. |
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 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. |
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 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? |
… to break compatibility
8754121
to
f231859
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.
Thanks!
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.