-
Notifications
You must be signed in to change notification settings - Fork 416
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
Stabilize ExternalDocumentableProvider
#3312
Conversation
/** | ||
* TODO [external-documentable-provider] add documentation | ||
*/ | ||
public interface ExternalDocumentableProvider { |
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.
In a few words, this extension is used in dackka to create documentables for supertypes that come from dependencies, because, if I'm not mistaken, by default Dokka only creates the documentable model for the code it finds in the project itself, but not its classpath.
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.
BTW, what about typealiases? They are not DClasslike
but could be supertypes, and so do we need to support them?
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.
It could indeed be useful, but we'd have to think about the corner cases, write tests for it and make sure the implementations support it, and at this point it doesn't seem to be needed, at least not for #3112. If someone does come with such a request, we can certainly consider it
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.
BTW, what about typealiases? They are not DClasslike but could be supertypes, and so do we need to support them?
AFAIK, dokka-devsite-plugin can use TypeAliased
that has inner
.
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.
Creating this PR as a draft because I still need to write documentation for the new API, but the review can already start as I don't plan to introduce any significant code changes.
In my opinion, an ideal review is not only about code.
Is this just renaming internal.ExternalDocumentablesProvider
into ExternalDocumentableProvider
?
import org.jetbrains.dokka.model.* | ||
import kotlin.test.* | ||
|
||
class ExternalDocumentableProviderTest { |
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.
Is this yet another set of tests for external documentable?
What are the plans for ExternalDocumentablesTest
? It also contains interesting corner cases.
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.
Deleted the old tests, as the new tests should cover all sensible corner cases I could think of
c6d0cce
to
24faa54
Compare
c5cd408
to
6a7dac9
Compare
Not only that, but adding comprehensible documentation and unit tests as well. The implementation didn't change though. |
Had to rebase the branch onto master and resolve some merge conflicts manually, so I squashed the commits and force pushed it. It's ready for review now |
…umentable-provider # Conflicts: # dokka-subprojects/analysis-kotlin-symbols/src/main/kotlin/org/jetbrains/dokka/analysis/kotlin/symbols/services/SymbolExternalDocumentablesProvider.kt
Part of #3112, depends on #3195
Creating this PR as a draft because I still need to write documentation for the new API, but the review can already start as I don't plan to introduce any significant code changes.