Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove modifier rendering if it empty #3343

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Remove modifier rendering if it empty #3343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3fc052e
Remove modifier rendering if it empty
atyrin Nov 14, 2023
cc459b0
Remove variance modifier rendering if it absent
atyrin Nov 14, 2023
653df6f
Add test for removed modifier rendering
atyrin Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions ...lugin-base/src/main/kotlin/org/jetbrains/dokka/base/signatures/KotlinSignatureProvider.kt
View file
Open in desktop
Expand Up @@ -248,7 +248,7 @@ public class KotlinSignatureProvider(
p.modifier[sourceSet].takeIf { it !in ignoredModifiers }?.let {
if (it is JavaModifier.Empty) KotlinModifier.Open else it
}?.name?.let { keyword("$it ") }
p.modifiers()[sourceSet]?.toSignatureString()?.let { keyword(it) }
p.modifiers()[sourceSet]?.toSignatureString()?.takeIf { it.isNotEmpty() }?.let { keyword(it) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can just return null from toSignatureString (used in 12 places with similar pattern) or even don't add any text in PageContentBuilder.DocumentableContentBuilder#text if the text is empty? So to fix all empty spans :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️ I've just added one more change in place where the toSignatureString is not used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, it should indeed be fixed once and for all, with some generic tests that check we have no empty spans/text at all.

However, this place and API is very rarely touched, there's a non-zero chance of new rendering bugs or corner cases appearing (especially if changes are made in DocumentableContentBuilder#text), and the empty span bugs are very minor and not noticeable by the users.

I'm afraid it might take way more time than what it's worth for now, so I'd focus on just removing obstacles for testing K2 and simplifying the diff. The rest can be done later during downtime, other refactorings or if we have nothing better to do :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, better not to do it now. Thought, may be we should at least create issue to track this? Something like Remove empty spans/tags in HTML? (interesting, are other formatters affected?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created that one: #3344
Feel free to rewrite and add possible areas of improvement.

if (p.isMutable()) keyword("var ") else keyword("val ")
list(p.generics, prefix = "<", suffix = "> ",
separatorStyles = mainStyles + TokenStyle.Punctuation,
Expand Down Expand Up @@ -303,7 +303,7 @@ public class KotlinSignatureProvider(
f.modifier[sourceSet]?.takeIf { it !in ignoredModifiers }?.let {
if (it is JavaModifier.Empty) KotlinModifier.Open else it
}?.name?.let { keyword("$it ") }
f.modifiers()[sourceSet]?.toSignatureString()?.let { keyword(it) }
f.modifiers()[sourceSet]?.toSignatureString()?.takeIf { it.isNotEmpty() }?.let { keyword(it) }
keyword("fun ")
list(
f.generics, prefix = "<", suffix = "> ",
Expand Down Expand Up @@ -434,7 +434,7 @@ public class KotlinSignatureProvider(
}

is Variance<*> -> group(styles = emptySet()) {
keyword("$p ".takeIf { it.isNotBlank() } ?: "")
p.takeIf { it.toString().isNotEmpty() }?.let { keyword("$it ") }
signatureForProjection(p.inner, showFullyQualifiedName)
}

Expand Down
66 changes: 64 additions & 2 deletions dokka-subprojects/plugin-base/src/test/kotlin/signatures/SignatureTest.kt
View file
Open in desktop
Expand Up @@ -197,6 +197,27 @@ class SignatureTest : BaseAbstractTest() {
}
}

@Test
fun `fun with use site variance modifier in`() {
val source = source("fun simpleFun(params: Array<in String>): Unit")
val writerPlugin = TestOutputWriterPlugin()

testInline(
source,
configuration,
pluginOverrides = listOf(writerPlugin)
) {
renderingStage = { _, _ ->
writerPlugin.writer.renderedContent("root/example/simple-fun.html").firstSignature().match(
"fun ", A("simpleFun"), "(", Parameters(
Parameter("params: ", A("Array"), "<in ", A("String"), ">"),
), ")",
ignoreSpanWithTokenStyle = true
)
}
}
}

@Test
fun `fun with definitely non-nullable types`() {
val source = source("fun <T> elvisLike(x: T, y: T & Any): T & Any = x ?: y")
Expand Down Expand Up @@ -312,6 +333,48 @@ class SignatureTest : BaseAbstractTest() {
}
}

@Test
fun `class with declaration site variance modifier`() {
val writerPlugin = TestOutputWriterPlugin()

testInline(
"""
|/src/main/kotlin/common/Test.kt
|package example
|
|class PrimaryConstructorClass<out T> { }
""".trimMargin(),
configuration,
pluginOverrides = listOf(writerPlugin)
) {
renderingStage = { _, _ ->
writerPlugin.writer.renderedContent("root/example/-primary-constructor-class/index.html").firstSignature().match(
Span("class "), A("PrimaryConstructorClass"), Span("<"), Span("out "), A("T"), Span(">"),
)
}
}
}

@Test
fun `constructor property on class page`() {
val source = source("data class DataClass(val arg: String)")
val writerPlugin = TestOutputWriterPlugin()

testInline(
source,
configuration,
pluginOverrides = listOf(writerPlugin)
) {
renderingStage = { _, _ ->
assertEquals(
writerPlugin.writer.renderedContent("root/example/-data-class/index.html").lastSignature().html(),
"<span class=\"token keyword\">val </span><a href=\"arg.html\">arg</a><span class=\"token operator\">: </span><a href=\"https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/-string/index.html\">String</a>"

)
}
}
}

@Test
fun `functional interface`() {
val source = source("fun interface KRunnable")
Expand Down Expand Up @@ -896,8 +959,7 @@ class SignatureTest : BaseAbstractTest() {
) {
renderingStage = { _, _ ->
writerPlugin.writer.renderedContent("root/example/-primary-constructor-class/index.html").firstSignature().match(
// In `<T>` expression, an empty `<span class="token keyword"></span>` is present for some reason
Span("class "), A("PrimaryConstructorClass"), Span("<"), Span(), A("T"), Span(">"), Span("("), Parameters(
Span("class "), A("PrimaryConstructorClass"), Span("<"), A("T"), Span(">"), Span("("), Parameters(
Parameter(Span("val "), "x", Span(": "), A("Int"), Span(",")),
Parameter(Span("var "), "s", Span(": "), A("String"))
), Span(")"),
Expand Down