Skip to content
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

Please allow non-Bloop BSPs to enable run/test code lens support #183

Closed
terpstra opened this issue Jan 22, 2021 · 4 comments
Closed

Please allow non-Bloop BSPs to enable run/test code lens support #183

terpstra opened this issue Jan 22, 2021 · 4 comments
Labels

Comments

@terpstra
Copy link

terpstra commented Jan 22, 2021

Is your feature request related to a problem? Please describe.

scala-metals only enables the run/debug code lens for bloop.

Describe the solution you'd like

If a BSP reports non-empty scalaMainClasses or scalaTestClasses, bloop should believe that the tool will support the run and test BSP methods for those classes. If scala-metals feels it cannot trust BSP implementations to properly implement the BSP, at least allow a field in .bsp/xyz.json to provide a mechanism for project maintainers to declare that this feature DOES work with the named BSP implementation.

Describe alternatives you've considered

Not using scala-metals for unit tests.

Additional context

If one of my above two proposals is deemed acceptable, I'd be happy to prepare a scala-metals PR which implements this feature myself.

Search terms:

run debug code lens bsp

@ckipp01
Copy link
Member

ckipp01 commented Jan 23, 2021

Thanks for the request @terpstra!

scala-metals only enables the run/debug code lens for bloop.

Currently yes, and it's something that we are aware of. Part of the tricky part is there is currently no way that is part of the spec to see if a server is a DAP provider. There is actual a request for this here:

build-server-protocol/build-server-protocol#145

It's something I've wanted to work on, but haven't found the time. Having this would allow for a reliable way to know if a server supports this or not. We made this change when sbt made their BSP integration because code lenses were being generated on Main methods when using sbt BSP, but they would do nothing since sbt didn't provide DAP support. It was for this reason that we added this:

https://github.com/scalameta/metals/blob/94a8c9c39f5f675cac279051d561c829776ac3b4/metals/src/main/scala/scala/meta/internal/metals/codelenses/RunTestCodeLens.scala#L23-L42

I'll admit, it's not ideal, but at the time there were no other BSP servers that were reported being a DAP provider, so it hasn't been an issue.

If a BSP reports non-empty scalaMainClasses or scalaTestClasses, bloop should believe that the tool will support the run and test BSP methods for those classes.

I'm not sure I understand "bloop should believe that the tool will support the run and test BSP methods for those classes". Do you mean metals here?

If so, if I'm remembering correctly, this may not always be the case. For example a server may know how to answer and return scalaMainClasses, but not be a debug provider. So we can't rely on if those are returned, then we can generate the code lenses.

provide a mechanism for project maintainers to declare that this feature DOES work with the named BSP implementation.

Again, this is sort of a limitation in the BSP spec. See the issue above.

@terpstra
Copy link
Author

terpstra commented Jan 23, 2021

If a BSP reports non-empty scalaMainClasses or scalaTestClasses, bloop should believe that the tool will support the run and test BSP methods for those classes.

I'm not sure I understand "bloop should believe that the tool will support the run and test BSP methods for those classes". Do you mean metals here?

I did. Sorry for miswording.

If so, if I'm remembering correctly, this may not always be the case. For example a server may know how to answer and return scalaMainClasses, but not be a debug provider. So we can't rely on if those are returned, then we can generate the code lenses.

You can use the runProvider and testProvider fields of capabilities in a build/initialize response to decide this. Granted, it won't tell you about the debug provider status, but it will tell you about whether or not to at least include a 'run'/'test' button, just not the 'debug' variants.

provide a mechanism for project maintainers to declare that this feature DOES work with the named BSP implementation.

Again, this is sort of a limitation in the BSP spec. See the issue above.

While I agree using the BSP would be best, you could also extend the schema of .bsp/xyz.json to allow it to be declared there. Or is the .bsp/xyz.json format not under metals' control?

@terpstra
Copy link
Author

I'll note that I'm far less interested in the 'debug' version of the code lens than the 'run' and 'test' variants. For us, at least, debug was never going to work, since our tests are generally only created by the JVM and run outside of it.

For the non-debug options in the code lens, I believe that between (runProvider and scalaMainClasses) and (testProvider and scalaTestClasses) you have sufficient information to enable the non-debug run and test code lens features.

@ckipp01
Copy link
Member

ckipp01 commented Mar 18, 2022

We should be able to close this now since build-server-protocol/build-server-protocol#145 is closed and in Metals we check for buildTarget.getCapabilities.getCanDebug when creating the code lenses. So this should work as expected now as long as the BSP server declares this. If there are still issues feel free to comment back.

@ckipp01 ckipp01 closed this as completed Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants