all-modules-page: Validate cross-module links during resolution #2901
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The existing behavior was just taking the first module that includes the package name of a given reference, so this resulted in broken links when a package is split across multiple modules. This change checks to ensure the resolved file exists before choosing it as the target for the resolved link. Signed-off-by: Eddie Ringle <[email protected]>
pbajurko
reviewed
Jun 28, 2023
| val modulePath = context.configuration.outputDir.absolutePath | ||
| val validLink = resolvedLinks.firstOrNull { | ||
| val absolutePath = File(modulePath + it) | ||
| absolutePath.isFile |
There was a problem hiding this comment.
This checks assumes that dependee submodules have been already processed but that is upto process order
in all-modules-page plugin. For our project it lead to situation where same DRI was sometimes resolved and sometimes not.
I workaround it by checking paths against Dokka's output in submodules since partial tasks will be done before multimodule starts but that is not clean solution.
There was a problem hiding this comment.
I agree. Thank you for noticing it. The workaround is correct, but it should not depend on particular paths like build/dokka/htmlMultiModule or build/dokka/htmlPartial/. It would be great to use module.sourceOutputDirectory
vmishenev
reviewed
Nov 28, 2023
| val validLink = resolvedLinks.firstOrNull { | ||
| val absolutePath = File(modulePath + it) | ||
| absolutePath.isFile | ||
| } ?: return null |
There was a problem hiding this comment.
Will it not work for the non-local external package-list at all? See the example with the serialization library from External documentation links configuration. Can you fix it, please?
It is nice to fix the issue at least for cross-module links.
A check of existing external URLs online can be excessive and Dokka can be run without access to the Internet.
Also, I think the fix should check links only if resolvedLinks has more than one link. What do you think? It can help avoid unexpected behaviour with resolving cross-module links.
This fix is local and temporary. We have a long-term plan #3368.
| @@ -56,7 +56,7 @@ class ResolveLinkCommandResolutionTest : MultiModuleAbstractTest() { | |||
| } | |||
| } | |||
|
|
|||
| val contentFile = setup(link) | |||
| val contentFile = setup(link, testedDri) | |||
vmishenev
added a commit
that referenced
this pull request
Jan 11, 2024
Fixes #2272. There is a situation where 2 (or more) gradle modules have the same package and dokka doesn’t know where to link. E.g. The class com.example.classA is in moduleA and com.example.classB is in moduleB moduleC depends on moduleA and moduleB. In moduleC: there is a link [com.example.classB]. Having only package-list, dokka doesn’t know where to link, since the package com.example is in both modules. This is short-term and temporary solution of #3368 to cover a case of links between only local modules in a project. It is based on #2901 from @EddieRingle. It generates an external link depending on a check if the local link exists. The check works only when the number of possible resolved links are more than one. * all-modules-page: Validate cross-module links during resolution The existing behavior was just taking the first module that includes the package name of a given reference, so this resulted in broken links when a package is split across multiple modules. This change checks to ensure the resolved file exists before choosing it as the target for the resolved link. Signed-off-by: Eddie Ringle <[email protected]> * Check links only if resolved links has more than one link * Add integration test * Refactor * Add comment --------- Signed-off-by: Eddie Ringle <[email protected]> Co-authored-by: Eddie Ringle <[email protected]>
IgnatBeresnev
pushed a commit
that referenced
this pull request
Jan 11, 2024
Fixes #2272. There is a situation where 2 (or more) gradle modules have the same package and dokka doesn’t know where to link. E.g. The class com.example.classA is in moduleA and com.example.classB is in moduleB moduleC depends on moduleA and moduleB. In moduleC: there is a link [com.example.classB]. Having only package-list, dokka doesn’t know where to link, since the package com.example is in both modules. This is short-term and temporary solution of #3368 to cover a case of links between only local modules in a project. It is based on #2901 from @EddieRingle. It generates an external link depending on a check if the local link exists. The check works only when the number of possible resolved links are more than one. * all-modules-page: Validate cross-module links during resolution The existing behavior was just taking the first module that includes the package name of a given reference, so this resulted in broken links when a package is split across multiple modules. This change checks to ensure the resolved file exists before choosing it as the target for the resolved link. Signed-off-by: Eddie Ringle <[email protected]> * Check links only if resolved links has more than one link * Add integration test * Refactor * Add comment --------- Signed-off-by: Eddie Ringle <[email protected]> Co-authored-by: Eddie Ringle <[email protected]> (cherry picked from commit 6904571)
vmishenev
added a commit
that referenced
this pull request
Mar 20, 2024
Fixes #2272. There is a situation where 2 (or more) gradle modules have the same package and dokka doesn’t know where to link. E.g. The class com.example.classA is in moduleA and com.example.classB is in moduleB moduleC depends on moduleA and moduleB. In moduleC: there is a link [com.example.classB]. Having only package-list, dokka doesn’t know where to link, since the package com.example is in both modules. This is short-term and temporary solution of #3368 to cover a case of links between only local modules in a project. It is based on #2901 from @EddieRingle. It generates an external link depending on a check if the local link exists. The check works only when the number of possible resolved links are more than one. * all-modules-page: Validate cross-module links during resolution The existing behavior was just taking the first module that includes the package name of a given reference, so this resulted in broken links when a package is split across multiple modules. This change checks to ensure the resolved file exists before choosing it as the target for the resolved link. Signed-off-by: Eddie Ringle <[email protected]> * Check links only if resolved links has more than one link * Add integration test * Refactor * Add comment --------- Signed-off-by: Eddie Ringle <[email protected]> Co-authored-by: Eddie Ringle <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I scanned the list of open issues but didn't spot one that matched my experience directly, but the closest might be #2037 or #2272. I'm not sure if this is necessarily the correct or ideal fix, and the underlying wider issue of supporting split packages probably requires further changes, but I did want to at least share this as it makes Dokka usable for our use-cases.
From commit message: