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
feat: find companion object of AnyVal, implicit class in class finder #4974
Conversation
mangledClassName: MangledClassName, | ||
shortClassName: ShortClassName, | ||
extensionSuffix: 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.
after some time, I had no idea what friendlyName
and description
are. MangledClassName
and ShortClassName
have defined meaning which makes it easier to see what's going on
object ClassFinderGranularity { | ||
case object ClassFiles extends ClassFinderGranularity | ||
case object Tasty extends ClassFinderGranularity |
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.
better tha boolean, isn't it?
@@ -178,6 +178,6 @@ class ClassBreakpointSuite extends BaseClassFinderSuite { | |||
buffers.put(path, sourceText) | |||
val sym = classFinder.findClass(path, pos.toLsp.getStart()) | |||
assert(sym.isDefined) | |||
assertNoDiff(sym.get, expected) | |||
assertNoDiff(sym.get.value, expected) |
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 suite is relevant anymore, since breakpoints are handled within the debug-adapter-library
. We could probably move everything to one suite and remove the public methods
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.
So both findClass
and findTasty
methods can be removed and test suites can be removed too?
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.
Super minor comment, but otherwise LGTM!
|
||
class ClassFinder(trees: Trees) { | ||
|
||
def findClass(path: AbsolutePath, pos: l.Position): Option[String] = | ||
findClass(path, pos, checkInnerClasses = true) | ||
def findClass(path: AbsolutePath, pos: l.Position): Option[MangledClassName] = |
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.
These two methods seem to only be used in tests 🤔
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.
#4974 (comment)
If I got you correctly we can remove test suites and then those methods, right?
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 so, unless some of those cases are not tested in FindAllClassesSuite
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'll create an issue about checking usage of this method, after it's created, I'll merge PR.
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.
Issue created #5071, I'm merging this PR
classes like
implicit class FooOps(private val foo: Int) extends AnyVal
produces two class files: FooOps.class and FooOps$.class. This PR: