fix: Fix reversed version order in dep completions in scala 2 #5206
Conversation
jkciesluk
force-pushed
the
dep-version-sort
branch
from
0d559b0
to
7a69920
Compare
mtags/src/main/scala-2/scala/meta/internal/pc/completions/ScalaCliCompletions.scala
Outdated
Show resolved
Hide resolved
| @@ -34,6 +34,16 @@ trait ScalaCliCompletions { | |||
| dependency: String | |||
| ) extends CompletionPosition { | |||
|
|
|||
| override def compare(o1: Member, o2: Member): Int = | |||
| (o1, o2) match { | |||
| case (c1: TextEditMember, c2: TextEditMember) => | |||
There was a problem hiding this comment.
Would it make sense to create DependencyMember and use SemVer class we have to compare?
There was a problem hiding this comment.
What abou this? Did you manage to get this working?
There was a problem hiding this comment.
I've added DependencyMember and reused it also in completions for sbt and mill (I've completely forgotten about them before).
jkciesluk
force-pushed
the
dep-version-sort
branch
from
7a69920
to
cd03288
Compare
Some libraries have versions in format like `2.5-<commit_hash>`. Now we can create Version for them. This allows us to eg. sort them corretly in completions
jkciesluk
force-pushed
the
dep-version-sort
branch
from
cd03288
to
8706e09
Compare
mtags-shared/src/main/scala/scala/meta/internal/semver/SemVer.scala
Outdated
Show resolved
Hide resolved
| @@ -132,6 +132,18 @@ class CompletionScalaCliSuite extends BaseCompletionSuite { | |||
| "circe-core_native0.4", | |||
| ) | |||
|
|
|||
| check( | |||
| "version-sort", | |||
There was a problem hiding this comment.
I remember you talking about some problematic cases, maybe add those as a test too.
There was a problem hiding this comment.
This is alternative-sorting test, it will fail with exception if if can't create Version because of non-standard version syntax
There was a problem hiding this comment.
Btw. you could add unit tests isntead to cover a wider amount of cases and it will be easy to add tests there.
There was a problem hiding this comment.
Can we add unit tests after solving #5300? Right now we would have to change from double colon to single colon in tests, and I'm not sure if thats fine
mtags-shared/src/main/scala/scala/meta/internal/semver/SemVer.scala
Outdated
Show resolved
Hide resolved
| @@ -132,6 +132,18 @@ class CompletionScalaCliSuite extends BaseCompletionSuite { | |||
| "circe-core_native0.4", | |||
| ) | |||
|
|
|||
| check( | |||
| "version-sort", | |||
There was a problem hiding this comment.
Btw. you could add unit tests isntead to cover a wider amount of cases and it will be easy to add tests there.
| @@ -34,6 +34,16 @@ trait ScalaCliCompletions { | |||
| dependency: String | |||
| ) extends CompletionPosition { | |||
|
|
|||
| override def compare(o1: Member, o2: Member): Int = | |||
| (o1, o2) match { | |||
| case (c1: TextEditMember, c2: TextEditMember) => | |||
There was a problem hiding this comment.
What abou this? Did you manage to get this working?
mtags/src/main/scala-2/scala/meta/internal/pc/completions/Completions.scala
Outdated
Show resolved
Hide resolved
Adds common class for all dependency completions
jkciesluk
force-pushed
the
dep-version-sort
branch
from
c6979f2
to
5ec09b4
Compare
In Scala 2 completions for dependencies where sorted in wrong order