Add option to suppress annotated declarations#4464
Add option to suppress annotated declarations#4464rnett wants to merge 10 commits intoKotlin:masterfrom
Conversation
whyoleg
left a comment
whyoleg
left a comment
There was a problem hiding this comment.
Thanks for the PR!
First round of review.
dokka-subprojects/core/src/main/kotlin/org/jetbrains/dokka/defaultConfiguration.kt
Outdated
Show resolved
Hide resolved
dokka-subprojects/core/src/main/kotlin/org/jetbrains/dokka/configuration.kt
Outdated
Show resolved
Hide resolved
...-api/src/main/kotlin/org/jetbrains/dokka/testApi/testRunner/TestDokkaConfigurationBuilder.kt
Outdated
Show resolved
Hide resolved
...in/kotlin/org/jetbrains/dokka/base/transformers/documentables/AnnotationSuppressionFilter.kt
Outdated
Show resolved
Hide resolved
...in/kotlin/org/jetbrains/dokka/base/transformers/documentables/AnnotationSuppressionFilter.kt
Outdated
Show resolved
Hide resolved
...in/kotlin/org/jetbrains/dokka/base/transformers/documentables/AnnotationSuppressionFilter.kt
Outdated
Show resolved
Hide resolved
dokka-subprojects/plugin-base/src/main/kotlin/org/jetbrains/dokka/base/DokkaBase.kt
Outdated
Show resolved
Hide resolved
whyoleg
left a comment
whyoleg
left a comment
There was a problem hiding this comment.
LGTM!
Let's also wait for @vmishenev's review.
dokka-subprojects/core/src/main/kotlin/org/jetbrains/dokka/defaultConfiguration.kt
Outdated
Show resolved
Hide resolved
vmishenev
left a comment
vmishenev
left a comment
There was a problem hiding this comment.
I only have a product-related question.
| | `noStdlibLink` | Whether to generate links to the Kotlin standard library. | | ||
| | `noJdkLink` | Whether to generate links to JDK Javadocs. | | ||
| | `suppressedFiles` | Paths to files to be suppressed. Accepts multiple paths separated by semicolons. | | ||
| | `suppressedAnnotations` | Annotation fully qualified names (FQNs) to suppress declarations annotated with. Accepts multiple values separated by semicolons. | |
There was a problem hiding this comment.
The naming is ambiguous.
By the name, I would expect this to work like suppressedFiles, meaning it should suppress annotations only, but it does not.
Maybe you have a better name.
There was a problem hiding this comment.
Yeah, I agree, I just couldn't think of anything better that wasn't super verbose. supressAnnotatedWith was the next closest.
There was a problem hiding this comment.
Yeah, I agree too, thanks for bringing this up!
I do like the suppressAnnotatedWith naming
| context: DokkaContext | ||
| ) : SuppressedByConditionDocumentableFilterTransformer(context) { | ||
|
|
||
| override fun shouldBeSuppressed(d: Documentable): Boolean { |
There was a problem hiding this comment.
There is a corner case in a KMP project
// jvm platform
@Supress
actual fun f()
// native platform
actual fun f()
In this case, both declarations will be filtered out.
Meanwhile, suppressedAnnotations is one of the source set options.
As for me, this case is OK for the current PR.
There was a problem hiding this comment.
In this case, both declarations will be filtered out.
I think we could at least have a test for this case, because it's not really obvious to me why this could happen.
We do run this filter in preMergeDocumentableTransformer phase, so I would expect it to be filtered out only in jvm source-set.
In some way, it reminds me of #3633 (though, there we filter out expect and not actual, but still), so we should handle such cases correctly during the merge.
@rnett, could you add such a test?
| documentablesMergingStage = { module -> | ||
| val pkg = module.packages.single { it.name == "test" } | ||
| val classNames = pkg.classlikes.map { it.name } | ||
| // SuppressMe itself should be present, but Annotated should be suppressed |
There was a problem hiding this comment.
Also, should we suppress the annotations themselves?
It seems that having them documented does not make sense in most cases.
There was a problem hiding this comment.
IMO that makes sense. @whyoleg thoughts? It would make supressedAnnotations work a bit better as the name.
There was a problem hiding this comment.
I don't think we should do this for multiple reasons.
One of them is that annotations could come from other libraries; e.g., there is even a mention of the compose @Preview annotation in the issue itself (#1861 (comment)).
The other one is composability.
We already have other mechanisms to suppress different things:
- MustBeDocumented - to not show annotation in generated documentation.
@suppresstag - to suppress declarations.suppressedFiles- to suppress all declarations in files.- per package
suppressflag - to suppress all declarations in the package.
So, I think that this new option (suppressAnnotatedWith) should also do only one thing - suppress annotated declarations, in line with all other suppress related mechanisms.
If annotation should also be suppressed, it could be easily done via single @suppress tag on that annotation.
Though we could mention that the @suppress tag can be used to hide the annotation itself in the kdoc/documentation of this new option, so it will be easier to discover.
WDYT?
3 participants
Fixes #1861
Adds a
suppressedAnnotationssource set level config that causes any declarations annotated with one of the given FqNames to be omitted from the documentation.