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

Fix wrong ordering of platforms #1483

Closed
wants to merge 1 commit into from
Closed

Fix wrong ordering of platforms #1483

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/template/src/main/twirl/scaladex/view/project/artifactsFilters.scala.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

@adpi2 adpi2 Oct 18, 2024 •
edited
Loading

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.

Copy link
Collaborator Author

@ayushkoli772 ayushkoli772 Oct 18, 2024 •
edited
Loading

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?

Copy link
Member

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:

Suggested change
@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.

<optgroup label="@platform">
@for(binaryVersion <- binaryVersions.reverse) {
<option value="@binaryVersion.label"
Expand Down