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

Make javadoc pages generation deterministic #2479

Merged
merged 2 commits into from May 5, 2022
Merged

Make javadoc pages generation deterministic #2479

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bbcc20c
Make javadoc pages generation deterministic
IgnatBeresnev Apr 28, 2022
b6c3d65
Rename `genericQuery` to avoid confusion with Generics
IgnatBeresnev May 5, 2022
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
1 change: 0 additions & 1 deletion plugins/javadoc/api/javadoc.api
View file
Open in desktop
Expand Up @@ -119,7 +119,6 @@ public final class org/jetbrains/dokka/javadoc/pages/DeprecatedPageSection : jav
public final fun getCaption ()Ljava/lang/String;
public final fun getHeader ()Ljava/lang/String;
public final fun getId ()Ljava/lang/String;
public final fun getPriority ()I
public static fun valueOf (Ljava/lang/String;)Lorg/jetbrains/dokka/javadoc/pages/DeprecatedPageSection;
public static fun values ()[Lorg/jetbrains/dokka/javadoc/pages/DeprecatedPageSection;
}
Expand Down
10 changes: 6 additions & 4 deletions plugins/javadoc/src/main/kotlin/org/jetbrains/dokka/javadoc/pages/JavadocPageNodes.kt
View file
Open in desktop
Expand Up @@ -317,17 +317,19 @@ class DeprecatedNode(val name: String, val address: DRI, val description: List<C
override fun hashCode(): Int = address.hashCode()
}

enum class DeprecatedPageSection(val id: String, val caption: String, val header: String, val priority: Int = 100) {
DeprecatedForRemoval("forRemoval", "For Removal", "Element", priority = 90),
enum class DeprecatedPageSection(val id: String, val caption: String, val header: String) {
DeprecatedModules("module", "Modules", "Module"),
DeprecatedInterfaces("interface", "Interfaces", "Interface"),
DeprecatedClasses("class", "Classes", "Class"),
DeprecatedEnums("enum", "Enums", "Enum"),
DeprecatedExceptions("exception", "Exceptions", "Exceptions"),
DeprecatedFields("field", "Fields", "Field"),
DeprecatedMethods("method", "Methods", "Method"),
DeprecatedConstructors("constructor", "Constructors", "Constructor"),
DeprecatedEnumConstants("enum.constant", "Enum Constants", "Enum Constant")
DeprecatedEnums("enum", "Enums", "Enum"),
DeprecatedEnumConstants("enum.constant", "Enum Constants", "Enum Constant"),
DeprecatedForRemoval("forRemoval", "For Removal", "Element");

internal fun getPosition() = ordinal
}

class IndexPage(
Expand Down
27 changes: 24 additions & 3 deletions plugins/javadoc/src/main/kotlin/org/jetbrains/dokka/javadoc/pages/htmlPreprocessors.kt
View file
Open in desktop
Expand Up @@ -90,9 +90,30 @@ object IndexGenerator : PageTransformer {
}
val keys = elements.keys.sortedBy { it }
val sortedElements = elements.entries.sortedBy { (a, _) -> a }
return input.modified(children = input.children + sortedElements.mapIndexed { i, (_, set) ->
IndexPage(i + 1, set.sortedBy { it.getId().toLowerCase() }, keys, input.sourceSets())
})

val indexNodeComparator = compareBy<NavigableJavadocNode> { it.getId().toLowerCase() }
.thenBy { it.getFullComparatorKey() }

val indexPages = sortedElements.mapIndexed { idx, (_, set) ->
IndexPage(
id = idx + 1,
elements = set.sortedWith(indexNodeComparator),
keys = keys,
sourceSet = input.sourceSets()
)
}
return input.modified(children = input.children + indexPages)
}

private fun NavigableJavadocNode.getFullComparatorKey(): String {
return getDRI().let { dri ->
Copy link
Member

Choose a reason for hiding this comment

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

DRI.toString is the same logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's generally bad to build business logic around and rely on .toString(). It's good for debugging, but very error-prone and difficult to get rid of in pretty much all other cases

Ideally, I'd actually like to get rid of all DRI.toString calls and replace them with some other method that you can easily find usages of

Copy link
Member

Choose a reason for hiding this comment

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

It's generally bad to build business logic around and rely on .toString()

Yes, so an comparator of DRI should be here.

val packageName = dri.packageName.orEmpty()
val className = dri.classNames.orEmpty()
val callableName = dri.callable?.name.orEmpty()
val parameters = dri.callable?.signature().orEmpty()

"$packageName/$className/$callableName/$parameters"
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...main/kotlin/org/jetbrains/dokka/javadoc/renderer/JavadocContentToTemplateMapTranslator.kt
View file
Open in desktop
Expand Up @@ -81,7 +81,7 @@ internal class JavadocContentToTemplateMapTranslator(
"id" to node.name,
"title" to "Deprecated",
"kind" to "deprecated",
"sections" to node.elements.toList().sortedBy { (section, _) -> section.priority }
"sections" to node.elements.toList().sortedBy { (section, _) -> section.getPosition() }
.map { (s, e) -> templateMapForDeprecatedPageSection(s, e) }
)

Expand Down
16 changes: 16 additions & 0 deletions plugins/javadoc/src/test/kotlin/org/jetbrains/dokka/javadoc/JavadocDeprecatedTest.kt
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ import org.jetbrains.dokka.javadoc.pages.DeprecatedPage
import org.jetbrains.dokka.javadoc.renderer.TemplateMap
import org.junit.jupiter.api.Assertions
import org.junit.jupiter.api.Test
import kotlin.test.assertEquals

internal class JavadocDeprecatedTest : AbstractJavadocTemplateMapTest() {

Expand Down Expand Up @@ -67,6 +68,21 @@ internal class JavadocDeprecatedTest : AbstractJavadocTemplateMapTest() {
}
}

@Test
fun `should be sorted by position`() {
testDeprecatedPageTemplateMaps { templateMap ->
@Suppress("UNCHECKED_CAST")
val contents = (templateMap["sections"] as List<TemplateMap>).map { it["caption"] }

// maybe some other ordering is required by the javadoc spec
// but it has to be deterministic
val expected = "Classes, Exceptions, Methods, Constructors, Enums, For Removal"
val actual = contents.joinToString(separator = ", ")

assertEquals(expected, actual)
}
}

@Test
fun `provides correct information for deprecated element`() {
testDeprecatedPageTemplateMaps { templateMap ->
Expand Down
140 changes: 89 additions & 51 deletions plugins/javadoc/src/test/kotlin/org/jetbrains/dokka/javadoc/JavadocIndexTest.kt
View file
Open in desktop
Expand Up @@ -2,14 +2,64 @@ package org.jetbrains.dokka.javadoc

import org.jetbrains.dokka.javadoc.pages.IndexPage
import org.jetbrains.dokka.javadoc.renderer.TemplateMap
import org.jetbrains.dokka.links.DRI
import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Test
import kotlin.test.assertNotNull

internal class JavadocIndexTest : AbstractJavadocTemplateMapTest() {

private val genericQuery = """
IgnatBeresnev marked this conversation as resolved.
Show resolved Hide resolved
/src/source0.kt
package package0
/**
* Documentation for ClassA
*/
class ClassA {
fun a() {}
fun b() {}
fun c() {}
}

/src/source1.kt
package package1
/**
* Documentation for ClassB
*/
class ClassB {
fun d() {}
fun e() {}
fun f() {}
}

/src/source2.kt
package package1
/**
* Documentation for ClassB
*/
class ClassC {
fun g() {}
fun h() {}
fun j() {}

class InnerClass {
fun k() {}
}
}

/src/source3.kt
package package1
/**
* Documentation for ClassCEnum
*/
enum class ClassCEnum {
A, D, E
}
""".trimIndent()

@Test
fun `generates correct number of index pages`() {
testIndexPages { indexPages ->
testIndexPages(genericQuery) { indexPages ->
assertEquals(12, indexPages.size)
}
}
Expand All @@ -20,14 +70,14 @@ internal class JavadocIndexTest : AbstractJavadocTemplateMapTest() {
fun hasAdditionalFunction() =
AnnotationTarget.ANNOTATION_CLASS::class.java.methods.any { it.name == "describeConstable" }

testIndexPages { indexPages ->
testIndexPages(genericQuery) { indexPages ->
assertEquals(if (hasAdditionalFunction()) 41 else 40, indexPages.sumBy { it.elements.size })
}
}

@Test
fun `templateMap for class index`() {
testIndexPagesTemplateMaps { templateMaps ->
testIndexPagesTemplateMaps(genericQuery) { templateMaps ->
@Suppress("UNCHECKED_CAST")
val element = (templateMaps[2]["elements"] as List<TemplateMap>)[1]
assertEquals("../package0/ClassA.html", element["address"])
Expand All @@ -41,7 +91,7 @@ internal class JavadocIndexTest : AbstractJavadocTemplateMapTest() {

@Test
fun `templateMap for enum entry index`() {
testIndexPagesTemplateMaps { templateMaps ->
testIndexPagesTemplateMaps(genericQuery) { templateMaps ->
@Suppress("UNCHECKED_CAST")
val element = (templateMaps[0]["elements"] as List<TemplateMap>).last()
assertEquals("../package1/ClassCEnum.html#A", element["address"])
Expand All @@ -55,7 +105,7 @@ internal class JavadocIndexTest : AbstractJavadocTemplateMapTest() {

@Test
fun `templateMap for function index`() {
testIndexPagesTemplateMaps { templateMaps ->
testIndexPagesTemplateMaps(genericQuery) { templateMaps ->
@Suppress("UNCHECKED_CAST")
val element = (templateMaps[0]["elements"] as List<TemplateMap>).first()
assertEquals("../package0/ClassA.html#a()", element["address"])
Expand All @@ -67,61 +117,49 @@ internal class JavadocIndexTest : AbstractJavadocTemplateMapTest() {
}
}

private val query = """
/src/source0.kt
package package0
/**
* Documentation for ClassA
*/
class ClassA {
fun a() {}
fun b() {}
fun c() {}
}

/src/source1.kt
package package1
/**
* Documentation for ClassB
*/
class ClassB {
fun d() {}
fun e() {}
fun f() {}
}

/src/source2.kt
package package1
/**
* Documentation for ClassB
*/
class ClassC {
fun g() {}
fun h() {}
fun j() {}

class InnerClass {
fun k() {}
}
@Test
fun `should sort overloaded functions deterministically`() {
val query = """
/src/overloaded.kt
package overloaded

class Clazz {
fun funName(param: List<String>) {}
fun funName(param: String) {}
fun funName(param: Map<String>) {}
fun funName(param: Int) {}
}
""".trimIndent()

/src/source3.kt
package package1
/**
* Documentation for ClassCEnum
*/
enum class ClassCEnum {
A, D, E
testIndexPages(query) { allPages ->
val indexPage = allPages.find { it.elements.any { el -> el.getId() == "funName" } }
assertNotNull(indexPage) { "Index page with functions not found" }

val indexElementDRIs = indexPage.elements.map { it.getDRI() }
assertEquals(4, indexElementDRIs.size)
indexElementDRIs.forEach {
assertEquals("overloaded", it.packageName)
assertEquals("Clazz", it.classNames)
assertEquals("funName", it.callable!!.name)
assertEquals(1, it.callable!!.params.size)
}
""".trimIndent()

private fun testIndexPages(operation: (List<IndexPage>) -> Unit) {
assertEquals("[kotlin.String]", indexElementDRIs[0].getParam(0))
assertEquals("kotlin.Int", indexElementDRIs[1].getParam(0))
assertEquals("kotlin.String", indexElementDRIs[2].getParam(0))
assertEquals("kotlin.collections.List[kotlin.String]", indexElementDRIs[3].getParam(0))
}
}

private fun DRI.getParam(index: Int) = this.callable!!.params[index].toString()

private fun testIndexPages(query: String, operation: (List<IndexPage>) -> Unit) {
testTemplateMapInline(query) {
operation(allPagesOfType())
}
}

private fun testIndexPagesTemplateMaps(operation: (List<TemplateMap>) -> Unit) =
private fun testIndexPagesTemplateMaps(query: String, operation: (List<TemplateMap>) -> Unit) =
testTemplateMapInline(query) {
operation(allPagesOfType<IndexPage>().map { it.templateMap })
}
Expand Down