Fix unstable ordering of methods #1394
Conversation
| @@ -129,7 +130,7 @@ open class DefaultPageCreator( | |||
| divergentBlock("Types", types, ContentKind.Classlikes, extra = mainExtra + SimpleAttr.header("Types")) | |||
| divergentBlock( | |||
| "Functions", | |||
| s.functions, | |||
| s.functions.sort(), | |||
There was a problem hiding this comment.
Just a question:
Would we maybe benefit from (whoever is containing this functions) already having the ordered the same way?
Maybe we are using the functions somewhere else and have unexpected unintuitive order somewhere? 🤔
There was a problem hiding this comment.
I agree and that was my first idea, but then after a discussion with @Kordyjan we decided to do it this way, as the presentation side is responsible for the correct displaying of declarations (and TBH it's faster this way as we don't have to modify the data structure)
| @@ -174,6 +175,9 @@ open class DefaultPageCreator( | |||
| } | |||
| } | |||
|
|
|||
| private fun Collection<DFunction>.sort() = | |||
There was a problem hiding this comment.
Extension might maybe be better on Iterable than Collection?
Would it make sense to let DFunction impelement Comaprable or is this order to specific to the current use case?
There was a problem hiding this comment.
Extension might maybe be better on Iterable than Collection?
Sure, changed 👍
Would it make sense to let DFunction impelement Comaprable or is this order to specific to the current use case?
We can do that, I have two issues with this approach though:
- It would be kind of inconsistent with other Documentables that don't implement
Comparable - The order here is introduced only for sensible presentation (eg. we prefer methods alphabetically with less arguments) and not for some sort of actual order. When implementing
ComparableI feel like one can assume that one method is bigger or smaller than other (as it would be then possible to dofun1 >= fun2) and it may lead to some wrong impressions based on different assumptions what bigger means.
Maybe we should rename sort to something like orderLexicographically() ?
There was a problem hiding this comment.
Very good arguments presented
I would be fine with calling it sort. Will merge this if you are either
kamildoleglo
force-pushed
the
fix-unstable-order
branch
from
b03346f
to
6c9f80c
Compare
kamildoleglo
force-pushed
the
fix-unstable-order
branch
from
6c9f80c
to
18a8e80
Compare
|
Cherry picked into 1.4.0 🍒 |
Closes #1333
This PR also fixes a bug in
SearchbarDataInstallerwhich had a race condition when used asynchronously fromHtmlRenderer