-
Notifications
You must be signed in to change notification settings - Fork 328
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: Add case
keyword completion
#5346
Conversation
You're so fast @jkciesluk! 🚀 |
d83b03e
to
4709e30
Compare
@@ -24,6 +24,7 @@ class CompletionCaseSuite extends BaseCompletionSuite { | |||
|}""".stripMargin, | |||
"""|case None => scala | |||
|case Some(value) => scala | |||
|case |
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 people would find it useful, this is suggested as a last option, so going down is almost the same amount of button clicks as just writing case
.
I would rather suggest this if we weren't able to suggest anything else.
How will this behave in the case that is in the linked issues?
Option(1).map{
c@@
case Some(a) =>
}
it would be awesome if None
was suggested. Having just case
would also be nice, but that would be a minimum I would expect to show up.
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.
I think we should only show a single case
if nothing else is shown to the user. Otherwise it's not that useful.
I think it already suggests None:
Do we have a test case for that? I thought this not working was the issue raised?
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 thought this not working was the issue raised?
I believe the issue was raised because there was no case
completions at all, but this happened because there was no extending classes, just objects with unapply
, which we don't check (maybe we could, by looking through object in scope with unapply method and correct argument?).
Do we have a test case for that?
No, I will add it.
I think we should only show a single case if nothing else is shown to the user.
Sure, we can do that
f4479ad
to
8632397
Compare
val label = "case" | ||
val suffix = | ||
if (clientSupportsSnippets) " $0 =>" | ||
else "" |
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'd add the space after case
anyway.
8632397
to
77e8f55
Compare
3c82024
to
ffdc195
Compare
ffdc195
to
4b38ed0
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
backports scalameta/metals#5346 [Cherry-picked f15bbda]
backports scalameta/metals#5346 [Cherry-picked f15bbda]
backports scalameta/metals#5346 [Cherry-picked f15bbda]
backports scalameta/metals#5346 [Cherry-picked f15bbda]
backports scalameta/metals#5346 [Cherry-picked f15bbda]
backports scalameta/metals#5346 [Cherry-picked f15bbda]
backports scalameta/metals#5346 [Cherry-picked f15bbda]
connected to #5339