Remove modifier rendering if it empty #3343

atyrin · 2023-11-14T09:41:37Z

For testing K2 migration, I'm using the approach of comparing Dokka output with K1. There are a lot of places where the difference is not user-visible but just an empty block <span class="token keyword"></span> in signatures. This makes it challenging to analyze the results. The PR should help to omit such spans.

IgnatBeresnev

👍 👍

vmishenev

Also, it would be nice to have a unit test.

whyoleg · 2023-11-14T11:29:01Z

...s/plugin-base/src/main/kotlin/org/jetbrains/dokka/base/signatures/KotlinSignatureProvider.kt

                        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) }


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 :)

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

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 :)

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?)

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

atyrin · 2023-11-14T13:01:54Z

@IgnatBeresnev @vmishenev please look once again. I've changed one more place for a similar case and added a few tests. Also, there is a suggestion from Oleg. Maybe we can address it with another PR later.

IgnatBeresnev

I think it's good to go as it addresses one specific problem: simplifies the diffs for testing K2.

It sucks to walk past the bad code you can easily improve, but I'd say fixing old technical debt falls outside the scope of this particular PR, as it wasn't even planned to begin with

whyoleg · 2023-11-14T14:47:35Z

...s/plugin-base/src/main/kotlin/org/jetbrains/dokka/base/signatures/KotlinSignatureProvider.kt

                        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) }


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?)

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [org.jetbrains.dokka](https://togithub.com/Kotlin/dokka) | plugin | patch | `1.9.10` -> `1.9.20` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>Kotlin/dokka (org.jetbrains.dokka)</summary> ### [`v1.9.20`](https://togithub.com/Kotlin/dokka/releases/tag/v1.9.20): 1.9.20 #### General bugfixes - Fixed sealed interfaces not having the `sealed` keyword in signatures ([https://github.com/Kotlin/dokka/issues/2994](https://togithub.com/Kotlin/dokka/issues/2994)) - Fixed incorrect links in multi-module projects with non-unique package names ([https://github.com/Kotlin/dokka/issues/2272](https://togithub.com/Kotlin/dokka/issues/2272)). Huge thanks to [@EddieRingle](https://togithub.com/EddieRingle)! - Fixed member extensions not being shown on index pages in certain scenarios ([https://github.com/Kotlin/dokka/issues/3187](https://togithub.com/Kotlin/dokka/issues/3187)) - Fixed Java's inner classes not having the `inner` keyword in Kotlin signatures ([https://github.com/Kotlin/dokka/issues/2793](https://togithub.com/Kotlin/dokka/issues/2793)) - Fixed Java's `@param` tag not working with type parameters ([https://github.com/Kotlin/dokka/issues/3199](https://togithub.com/Kotlin/dokka/issues/3199)) - Fixed Dokka failing in KMP projects when the JVM source set is suppressed ([https://github.com/Kotlin/dokka/issues/3209](https://togithub.com/Kotlin/dokka/issues/3209)) #### HTML format - Provide an ability to add a custom homepage link to the header, more details in [https://github.com/Kotlin/dokka/issues/2948#issuecomment-1976723089](https://togithub.com/Kotlin/dokka/issues/2948#issuecomment-1976723089) - Fixed tab selection resetting after navigating to a different page ([https://github.com/Kotlin/dokka/issues/2899](https://togithub.com/Kotlin/dokka/issues/2899)) - Fixed inline code not always being aligned with the surrounding text ([https://github.com/Kotlin/dokka/issues/3228](https://togithub.com/Kotlin/dokka/issues/3228)) - Fixed the "No options found" text in search being barely visible ([https://github.com/Kotlin/dokka/issues/3281](https://togithub.com/Kotlin/dokka/issues/3281)) - Fixed empty HTML tags being rendered for no reason ([https://github.com/Kotlin/dokka/pull/3343](https://togithub.com/Kotlin/dokka/pull/3343), [https://github.com/Kotlin/dokka/issues/3095](https://togithub.com/Kotlin/dokka/issues/3095)) #### Runners ##### Gradle Plugin - Mark tasks as not compatible with Gradle configuration cache, second try ([https://github.com/Kotlin/dokka/pull/3438](https://togithub.com/Kotlin/dokka/pull/3438)). Thanks to [@3flex](https://togithub.com/3flex) for noticing and fixing the problem! ##### Maven Plugin - Fixed `dokka:help` being absent ([https://github.com/Kotlin/dokka/issues/3035](https://togithub.com/Kotlin/dokka/issues/3035)). Thanks to [@aSemy](https://togithub.com/aSemy)! - Fixed the source links configuration not working ([https://github.com/Kotlin/dokka/pull/3046](https://togithub.com/Kotlin/dokka/pull/3046)). Thanks to [@freya022](https://togithub.com/freya022) for fixing this one! ##### CLI runner - Allow using relative paths in the `sourceRoots` configuration option ([https://github.com/Kotlin/dokka/issues/2571](https://togithub.com/Kotlin/dokka/issues/2571)) #### Plugin API - Provide an extension point to customize the rendering of code blocks in HTML format ([https://github.com/Kotlin/dokka/issues/3244](https://togithub.com/Kotlin/dokka/issues/3244)) #### Other: - Make sure `wasm-js` and `wasm-wasi` targets introduced in Kotlin 1.9.20 are supported ([https://github.com/Kotlin/dokka/issues/3310](https://togithub.com/Kotlin/dokka/issues/3310)) - Avoid concurrent invocations of Kotlin compiler's API due to the compiler API itself not always being thread safe ([

Remove modifier rendering if it empty

3fc052e

atyrin requested review from IgnatBeresnev and vmishenev November 14, 2023 09:41

IgnatBeresnev added this to the Dokka 1.9.20 milestone Nov 14, 2023

IgnatBeresnev assigned atyrin Nov 14, 2023

IgnatBeresnev approved these changes Nov 14, 2023

View reviewed changes

vmishenev approved these changes Nov 14, 2023

View reviewed changes

whyoleg reviewed Nov 14, 2023

View reviewed changes

atyrin added 2 commits November 14, 2023 13:11

Remove variance modifier rendering if it absent

cc459b0

Add test for removed modifier rendering

653df6f

atyrin requested review from IgnatBeresnev and vmishenev November 14, 2023 13:02

IgnatBeresnev approved these changes Nov 14, 2023

View reviewed changes

whyoleg approved these changes Nov 14, 2023

View reviewed changes

vmishenev approved these changes Nov 14, 2023

View reviewed changes

atyrin merged commit 94a4edd into master Nov 14, 2023

atyrin deleted the remove-empty-signature-spans branch November 14, 2023 15:35

atyrin mentioned this pull request Nov 14, 2023

Remove empty spans/tags in HTML #3344

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove modifier rendering if it empty #3343

Remove modifier rendering if it empty #3343

Uh oh!

atyrin commented Nov 14, 2023 •

edited

Loading

Uh oh!

IgnatBeresnev left a comment

Uh oh!

vmishenev left a comment

Uh oh!

whyoleg Nov 14, 2023

Uh oh!

atyrin Nov 14, 2023

Uh oh!

IgnatBeresnev Nov 14, 2023 •

edited

Loading

Uh oh!

whyoleg Nov 14, 2023

Uh oh!

atyrin Nov 14, 2023

Uh oh!

atyrin commented Nov 14, 2023

Uh oh!

IgnatBeresnev left a comment

Uh oh!

whyoleg Nov 14, 2023

Uh oh!

Remove modifier rendering if it empty #3343

Remove modifier rendering if it empty #3343

Uh oh!

Conversation

atyrin commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IgnatBeresnev left a comment

Choose a reason for hiding this comment

Uh oh!

vmishenev left a comment

Choose a reason for hiding this comment

Uh oh!

whyoleg Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

atyrin Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

IgnatBeresnev Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whyoleg Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

atyrin Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

atyrin commented Nov 14, 2023

Uh oh!

IgnatBeresnev left a comment

Choose a reason for hiding this comment

Uh oh!

whyoleg Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

atyrin commented Nov 14, 2023 •

edited

Loading

IgnatBeresnev Nov 14, 2023 •

edited

Loading