-
Notifications
You must be signed in to change notification settings - Fork 339
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
improvement: print better bracket suffix in label, Scala 3 completions #5497
Conversation
tests/cross/src/test/scala/tests/pc/CompletionWorkspaceSuite.scala
Outdated
Show resolved
Hide resolved
|""".stripMargin, | ||
"TTT[A] = O.TTT", | ||
compat = Map( | ||
"3" -> "TTT[A <: scala.Int] >: List[A] <: List[A]" |
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.
"3" -> "TTT[A <: scala.Int] >: List[A] <: List[A]" | |
"3" -> "TTT[A <: Int]" |
doesn't look like we need the other bounds 🤔
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.
The other bounds are on the result. Do we not want it? I guess we could instead of >: List[A] <: List[A]
write = List[A]
if the upper and lower bounds are the same.
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 think this might already be handled in Scala 2, which we could maybe reuse, but we don't want to show bounds if they are exactly the same, that brings no additional information to the user
mtags/src/main/scala-3/scala/meta/internal/pc/completions/CompletionValue.scala
Outdated
Show resolved
Hide resolved
mtags/src/main/scala-3/scala/meta/internal/pc/completions/CompletionValue.scala
Outdated
Show resolved
Hide resolved
c0166f1
to
31f1812
Compare
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.
LGTM
check( | ||
"type-with-params", | ||
s"""|object O { | ||
| type TTT[A <: Int] = List[A] |
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 wonder if this would be an issue with match types, whether we would not show too much for the user if we show the right hand side
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.
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.
Hmm... since it's in the detail maybe that's a boon? Let's leave it as is
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.
The question was if you want the detail to be
[type params] = rhs
as currently, or
rhs
or possibly
= rsh
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.
Maybe = rhs
?
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.
LGTM!
d5eff2d
to
a3f354c
Compare
This PR contains the following changes: 1. better printing of type params when there is a bracket suffix. We also print the bounds now and keep the original type param names. 2. we don't print the type params coming from bracket suffix in some situations, e.g. for types, where it is printed anyway 3. add completion suffix to `Scope` completions
6940edc
to
544b51c
Compare
544b51c
to
846f221
Compare
I ignored all the new/changed tests for dotty pc. Can I merge it like this, @tgodzik? |
Sure! We can also backport things to the compiler later on and update the tests afterwards. |
…8380) backport from metals: scalameta/metals#5497 CC: @tgodzik
This PR contains the following changes:
Scope
completions