-
Notifications
You must be signed in to change notification settings - Fork 76
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.
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
Fix wrong ordering of platforms #1483
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Ayush, I am glad to see you there again :)
@@ -15,7 +15,7 @@ | |||
<select class="selectpicker" name="binary-versions" | |||
title= "@params.binaryVersionsSummary.getOrElse("Scala Versions")" multiple data-style="btn-secondary" data-actions-box="true" data-selected-text-format="static" | |||
onchange="this.form.submit()"> | |||
@for((platform, binaryVersions) <- allBinaryVersions.sorted.groupBy(_.platform)) { | |||
@for((platform, binaryVersions) <- allBinaryVersions.sorted.groupBy(_.platform).toSeq.sortBy(_._1).reverse) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to sort it twice. The first .sorted
call should be enough, although it may need to be reversed. Otherwise we can remove it and only keep the sortBy(_._1).reverse
. But I think I would prefer to keep the first one.
EDIT: I am wrong here, the first .sorted
is useless because it is followed by .groupBy
which does not preserve the order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if im wrong.
First .sorted
will sort the binary versions and then the sortBy(_._1).reverse
will sort the grouped platforms. So output of this approach will be binary versions are in sorted order and the platforms will also be sorted that way Scala Native 0.5 will always appear before Scala Native 0.4.
Alternative to this approach will be sorting the grouped platforms based on the max version in each platform group and then sort the binary versions to show the latest first. It can be done by something like this- allBinaryVersions.groupBy(_.platform).toSeq .sortBy { binaryVersions.map(_.version).max }
Output of this approach will be binary versions are in sorted order and the platforms will be sorted based on which platform has latest/max binary version. So Scala Native 0.5 will be over Scala Native 0.4.
And also we dont need to sort it twice using alternate approach.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First .sorted will sort the binary versions and then the sortBy(_._1).reverse will sort the grouped platforms
The binary versions is composed of the platform and the language. So sorting the binary version should also sort the platforms and we should not need to sort them again. But then the .groupBy
creates a Map
which does not preserve the order. So I think the solution here is to remove the first sorted
:
@for((platform, binaryVersions) <- allBinaryVersions.sorted.groupBy(_.platform).toSeq.sortBy(_._1).reverse) { | |
@for((platform, binaryVersions) <- allBinaryVersions.groupBy(_.platform).toSeq.sortBy(_._1).reverse) { |
Alternative to this approach will be sorting the grouped platforms based on the max version in each platform group and then sort the binary versions to show the latest first.
There already is an Ordering[Platform]
defined as :
scaladex/modules/core/shared/src/main/scala/scaladex/core/model/Platform.scala
Lines 81 to 87 in a306d3c
implicit val ordering: Ordering[Platform] = Ordering.by { | |
case Jvm => (5, None) | |
case ScalaJs(version) => (4, Some(version)) | |
case ScalaNative(version) => (3, Some(version)) | |
case SbtPlugin(version) => (2, Some(version)) | |
case MillPlugin(version) => (1, Some(version)) | |
} |
We should better use it than redefining a custom one here.
Hello Adrien! Im glad to see you too :) |
Superseded by #1484 |
Changes to fix #1450 -
@adpi2 Please review