-
Notifications
You must be signed in to change notification settings - Fork 338
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 debug adapter for running main class to metals #6383
Conversation
@kasiaMarek It's great that Metals will be able to debug without the BSP server having to support it. In Why not use the BSP It wouldn't simplify running main classes much but then you can debug tests in the same way using |
2dfe4b4
to
a641cf6
Compare
Sorry for taking so long to write back, it slipped my mind.
We can do that but as you've noticed there isn't much of a benefit to it.
We need events, if we just run the test like this we'd only get the output. So it's not really an easier option. (But I feel like I only have a partial understanding of this so correct if I'm wrong.) |
Do you mean test events? You'd get those through BSP Test Report messages. Does Metals currently discover/run tests itself rather than delegating to BSP? I'd argue that it's the Build Server's responsibility to do this as it should already understand the test frameworks it supports and then Metals doesn't have to understand any test frameworks, however BSP doesn't allow test discovery at the method level (only class level) and I can't currently see how it's possible to obtain EDIT: I don't think there's a way to send back stacktraces of failed tests from BSP either. |
Oh, yes, that should in theory work. But besides the inability to run only test selection there seem to be other issues there. The biggest is that the run output gets send to
I think we simply always start debug server to run tests. |
Ah - I hadn't realised that the build tools don't even send back TestReports. Probably best to just handle it all within Metals I guess. |
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.
Great work! I added some mostly minor comments
metals/src/main/scala/scala/meta/internal/metals/debug/server/DebugeeParamsCreator.scala
Outdated
Show resolved
Hide resolved
|
||
val optClasspath = | ||
buildTargets | ||
.targetClasspath(id, cancelPromise) |
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.
.targetClasspath(id, cancelPromise) | |
.targetJarClasspath(id, cancelPromise) |
would not be good enough? Or is it due to the mill issue?
metals/src/main/scala/scala/meta/internal/metals/debug/server/MainClassDebugAdapter.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/debug/server/MainClassDebugAdapter.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/debug/server/MainClassDebugAdapter.scala
Outdated
Show resolved
Hide resolved
8a4e818
to
6b83c95
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.
Two minor comments otherwise LGTM
metals/src/main/scala/scala/meta/internal/metals/TargetData.scala
Outdated
Show resolved
Hide resolved
_ <- compilations.compileTargets(params.getTargets().asScala.toSeq) | ||
} yield { | ||
val debuggee = getDebugee.getOrElse( | ||
throw new RuntimeException(s"Can't resolve debugee") |
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.
This will most likely be thrown back to the user, so best to have some along the lines of Cannot resolve main class
etc. The more information the better.
748e6a3
to
d0fcaf5
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!
connected to: #5928
requires scalacenter/scala-debug-adapter#704 first