-
Notifications
You must be signed in to change notification settings - Fork 330
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
Add Hover Test and fix broken cases #55
Conversation
baa447d
to
86ede97
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.
A nice test suite made me so much more productive hacking on signature helper, awesome to see we can reuse the same infrastructure for hover 😸
@@ -217,4 +204,17 @@ object Compiler extends LazyLogging { | |||
f(r) | |||
r | |||
} | |||
def typeOfTree(c: Global)(t: c.Tree): Option[String] = { |
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.
In #51 I moved all logic into compiler.SignatureHelpProvider
, I think it would make sense to have a corresponding compiler.HoverProvider
object, even if the method is small.
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.
Yes, that's what I intend to do in this PR. I opened it for early feedback, but it's not quite ready for merge :) thanks for the review!
Ok, I'm fairly happy with this. I'm sure there's some trick cases we're not accounting for, but now it should be easy to quickly add a failing test and tweak |
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 👍
Next up: completion tests!
I discovered that #38 wasn't such a good idea. For some cases the "long string" isn't what we want on hover, e.g. for
the result is
a.x.type (with underlying type List[Int])
, whereas we would just likeList[Int]
.In this PR I've added a few tests (built on the infrastructure of #51) that also highlight the failing cases.
I intend to fix them in this PR and I think the simplest solution is to go back to using
.tpe.widen
and maybe handling a few edge cases manually.By the way, 👏 @olafurpg for the testing infra with chevrons: really handy to use!