Sort divergent elements deterministically #2846
Conversation
| compareBy<Documentable, String?>(nullsLast()) { it.dri.packageName } | ||
| .thenBy(nullsFirst()) { it.dri.classNames } // nullsFirst for top level to be first | ||
| .thenBy( | ||
| nullsLast( | ||
| compareBy<Callable> { it.params.size } | ||
| .thenBy { it.signature() } | ||
| ) | ||
| ) { it.dri.callable } |
There was a problem hiding this comment.
There's definitely room for improvement in terms of sorting rules, but it's a start. To me it makes sense to display top level declarations to be on top as they have a wider scope, and to sort vararg-like overloads (like these) in a straircase-like manner - both are implemented.
Tell me if you can think of anything else right off the bat
| @@ -187,4 +153,220 @@ class PageNodeMergerTest : BaseAbstractTest() { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| @RepeatedTest(3) | |||
| fun `should deterministically render same name property extensions`() { | |||
There was a problem hiding this comment.
I covered all the cases I could think of, but if you can think of a corner case that isn't covered or just a scenario you want to check - I'd be happy to add tests for it
| * @see DefaultPageCreator.sortDivergentElementsDeterministically for usage | ||
| */ | ||
| private val divergentDocumentableComparator = | ||
| compareBy<Documentable, String?>(nullsLast()) { it.dri.packageName } |
There was a problem hiding this comment.
It can be confusing that extensions from different packages are placed in the same group. Extensions from different packages have different pages, each own one, and the group title is a link that leads to the page of the first one.
It doesn't block this PR, but a consideration for the further improvement.
There was a problem hiding this comment.
Yeah, I also noticed it, but couldn't come up with how it can be fixed, other than not grouping such extensions under one block.
Hopefully, one day we will get rid of the left column altogether as planned, but in the meantime I can create an issue for fixing this, if you think it's significant enough.
There was a problem hiding this comment.
I haven't seen such case of extensions from different packages in stdlib so far, but it can happen in kotlinx.coroutines once #2845 is implemented.
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
| * @see DefaultPageCreator.sortDivergentElementsDeterministically for usage | ||
| */ | ||
| private val divergentDocumentableComparator = | ||
| compareBy<Documentable, String?>(nullsLast()) { it.dri.packageName } |
There was a problem hiding this comment.
Why is nullsLast for a package? should the "null" package be first?
There was a problem hiding this comment.
packageName is nullable, so it has to be either first or last :) By default comparison rules in this function, all nulls are last - the explicit use just locks it into place
should the "null" package be first?
If you have a reason for why it should, I don't mind
There was a problem hiding this comment.
Null for package means the top level
dokka/core/src/main/kotlin/links/DRI.kt
Line 24 in 683f6bb
There was a problem hiding this comment.
But I have checked we have an empty string for the root package
There was a problem hiding this comment.
It seems like the last question here is about null but it's unclear whether nulls are possible at all.
The reasonable outcomes might be the following:
- Just pick
nullsFirst()ornullsLast(), file an issue and investigate whethernulls are possible aspackageName - Figure out it here and fix accordingly.
Taking into account release pressure, my vote is for the first option
There was a problem hiding this comment.
it's unclear whether nulls are possible at all
Unless we're ready to change the API (i.e make packageName non nullable, mind 133 usages + it's very commonly used public API), I don't think it matters much - we'll still have to account for it one way or another. Unless you're proposing to add !! 🌚
So yeah, can be either first or last, I don't really care. I picked last for consistency with callable, which is also nullable and nullsLast(), but if there's a reason for why it should be first - I don't mind
There was a problem hiding this comment.
We have some stubs (as an empty string) for an unnamed package
If we do not break it, the sort should work (I hope).
So the first option is reasonable for me to merge the PR sooner.
I picked last for consistency with callable
But it is inconsistent with .thenBy(nullsFirst()) { it.dri.classNames } so I dropped attention to this.
There was a problem hiding this comment.
But it is inconsistent with
Yes, that's why .thenBy(nullsFirst()) { it.dri.classNames } has a comment to explain the decision
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Show resolved
Hide resolved
IgnatBeresnev
force-pushed
the
divergent-sort
branch
from
b1571b4
to
c1e0144
Compare
Fixes #2784