-
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
Simplify multi-module documentation #1387
Conversation
ee0ec51
to
d32a1bf
Compare
81d2a76
to
4cbe1fc
Compare
e57115e
to
17c4ab9
Compare
...gradle/src/integrationTest/kotlin/org/jetbrains/dokka/it/gradle/Collector0IntegrationTest.kt
Show resolved
Hide resolved
runners/gradle-plugin/src/main/kotlin/org/jetbrains/dokka/gradle/GradleDokkaSourceSetBuilder.kt
Outdated
Show resolved
Hide resolved
...base/src/main/kotlin/parsers/moduleAndPackage/parseModuleAndPackageDocumentationFragments.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/parsers/moduleAndPackage/ModuleAndPackageDocumentationSource.kt
Outdated
Show resolved
Hide resolved
c56e4f5
to
354e0af
Compare
Fix rebasing issue
…e concept of `moduleDisplayName`
7a124f5
to
50962e9
Compare
var pluginsClasspath: List<File> = emptyList() | ||
var pluginsConfigurations: Map<String, String> = emptyMap() | ||
var failOnWarning: Boolean = false | ||
private val lazySourceSets = mutableListOf<Lazy<DokkaSourceSetImpl>>() |
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 few things can be simplified if we were storing here not Lazy<DokkaSourceSetImpl>
but (moduleName: String) -> DokkaSourceSetImpl
. Especially we would get rid of strange restriction that module name must be defined before the sourcesets.
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.
Bump
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.
Oh. We should have continued this conversation here. Pawel and me talked about this in Slack:
Basically my argument here was, that making it the signature () -> DokkaSouceSetImpl would work, but it would just obfuscate the original contract which is: Yes this is lazy, but yes you will always get the same instance. IIRC, then we agreed on using Lazy here to signal this contract!
suppressedFiles = emptySet(), | ||
analysisPlatform = Platform.DEFAULT | ||
) | ||
val defaultSourceSet = testApi.testRunner.defaultSourceSet |
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.
Do we really need this file?
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.
We would not, but that would require me to change a lot usages to refer to the new path. I putted this into a "lower" module, since I needed it there 🙄
Do you want me to go ahead changing all usages, or should I go with a Deprecated annotation?
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.
Annotating it as deprecated would produce lots of warnings, wouldn't it?
I know that removing it will be a tedious process but for the sake of project we shouldn't have leftovers such as this in our repo. 😒
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.
Done ✅
Co-authored-by: Paweł Marks <[email protected]>
…in DModule and DPackage
Closes #1265