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

Suppress model-refresh if debugging is unsupported #1019

Conversation

@marek1840
Copy link
Collaborator

marek1840 commented Oct 25, 2019

closes #1016

Previously, model-refresh command was sent to the client even though it didn't support debugging.
Now, metals will refrain from sending in in such cases.

import scala.meta.internal.tvp._
import scala.meta.internal.metals.MetalsEnrichments._

trait MetalsLanguageClient extends LanguageClient with TreeViewClient {
@volatile private var debuggingSupported = true

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 25, 2019

Member

Any logic related to configuring what gets forwarded to the language client should be handled in ConfiguredLanguageClient.

As a general rule of thumb, I would recommend avoiding val and var inside traits.

@@ -494,7 +498,7 @@ final class TestingServer(
_ = client.refreshModelHandler = handler
// first compilation, to trigger the handler
_ <- server.compilations.compileFiles(List(path))
lenses <- codeLenses.future
lenses <- codeLenses.future.withTimeout(30, TimeUnit.SECONDS)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 25, 2019

Member

what is the motivation for this change? 30 seconds is a long timeout.

This comment has been minimized.

Copy link
@marek1840

marek1840 Oct 25, 2019

Author Collaborator

Indeed it is. But the timeout here prevents the whole test suite from hanging.
On the other hand, I can see now that there already is a maxDuration = 10 minutes for each async test case, so this particular change can probably be avoided

import scala.util.Failure
import scala.util.Success

object UnsupportedDebuggingLspSuite

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 25, 2019

Member

👍 Thank you for writing a test case!

There already was a configurable client which only had to be
adapted for configuration received from the client
@@ -93,7 +99,14 @@ final class ConfiguredLanguageClient(
params: ExecuteCommandParams
): Unit = {
if (config.executeClientCommand.isOn) {
underlying.metalsExecuteClientCommand(params)
params.getCommand match {
case _ if config.executeClientCommand.isOff =>

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 25, 2019

Member

Why is this line needed? We already guard against isOn above


languageClient.underlying match {
case client: ConfiguredLanguageClient =>
client.configure(clientExperimentalCapabilities)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 25, 2019

Member

Would it make sense to add this method to MetalsLanguageClient with a default implementation that ignores the argument?

@@ -39,9 +39,6 @@ abstract class NoopLanguageClient extends MetalsLanguageClient {
override def metalsTreeViewDidChange(
params: TreeViewDidChangeParams
): Unit = ()
// override def metalsTreeViewNodeReveal(

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 25, 2019

Member

Why is this change needed?

This comment has been minimized.

Copy link
@marek1840

marek1840 Oct 30, 2019

Author Collaborator

As a rule of thumb, I don't like to keep a commented out code in the codebase. If the code is unused, then it can and probably should be removed.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 30, 2019

Member

I agree! I thought the method was removed in this PR but I didn't see it was commented out.

@@ -35,9 +34,8 @@ object Main {
.setRemoteInterface(classOf[MetalsLanguageClient])
.setLocalService(server)
.create()
val underlyingClient = launcher.getRemoteProxy
val client = new ConfiguredLanguageClient(underlyingClient, config)(ec)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 25, 2019

Member

What was the motivation for this change?

This comment has been minimized.

Copy link
@marek1840

marek1840 Oct 30, 2019

Author Collaborator

The introduced UnsupportedDebuggingLspSuite tests the behavior of the ConfigurationLanguageClient which is responsible for suppressing the model-refresh notification. Without this change, we couldn't test it, as the ConfigurationLanguageClient was created only inside Main object, and then injected to the language server (hence not present in the test suite).

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 30, 2019

Member

I see, makes sense, thanks for the explanation!

marek1840 and others added 3 commits Oct 30, 2019
Co-Authored-By: Ólafur Páll Geirsson <olafurpg@gmail.com>
@marek1840 marek1840 force-pushed the marek1840:1016-suppress-model-refresh-if-unsupported branch from eb192db to 35345e7 Oct 30, 2019
@olafurpg olafurpg self-requested a review Oct 30, 2019
Copy link
Member

olafurpg left a comment

LGTM 👍 Thank you for addressing all of the comments!

@marek1840 marek1840 merged commit 220578c into scalameta:master Oct 30, 2019
9 checks passed
9 checks passed
Windows unit tests
Details
Linux unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Slow tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.