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

feature: Add the possibility to discover the main class to run #4968

Merged
merged 2 commits into from Feb 14, 2023

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Feb 10, 2023

Previously, the only possibility to discover the main class to run was to try and run it. Now, we can also ask for it so that client editor can use that shell command.

Needed for scalameta/metals-vscode#1298

Previously, the only possibility to discover the main class to run was to try and run it. Now, we can also ask for it so that client editor can use that shell command.
@@ -1825,6 +1826,10 @@ class MetalsLspService(
Option(params.uri).map(_.toAbsolutePath)
)
}.asJavaObject
case ServerCommands.DiscoverMainClasses(unresolvedParams) =>
Copy link
Member

@ckipp01 ckipp01 Feb 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing the context, but reading over the VS Code pr it wasn't clear. Isn't this essentially the same as

[Trace - 08:58:37 am] Received request 'workspace/executeCommand - (21)'
Params: {
  "command": "metals.debug-adapter-start",
  "arguments": [
    {
      "path": "file:///Users/ckipp/Documents/scala-workspace/minimal/minimal3/src/Main.scala",
      "runType": "runOrTestFile"
    }
  ]
}

Which already works to discover mains int he current file. I find this a bit confusing because now we have multiple different ways to discover main classes. Couldn't the existing runtType here with debug-adapter-start be re-utilized to do this same thing? If I'm understanding the usecase wouldn't adding a new runType of runTarget accomplish this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that the way we previously did this is by starting the session itself within Metals. Here we just want to get the command to run on the client. So we could potentially add a parameter (returnShellCommand: true or something along these lines), but then this flag would make the behaviour widely different. One would start app and return a address to connect to and the other would return the session object. I think we shouldn't to it in the same command

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this only adds a single new command, while reusing all the same mechanism, getting to work it with a flag might end up with considerable more effort.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh ok ok, make sense. Took some more time to really understand it. Thanks for explaining!

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright so just one more nit. Do you think we can also mention this in https://scalameta.org/metals/docs/integrations/debug-adapter-protocol and talk a bit about the intended flow. Mainly, in what situation would a client want to use the way that I questions about before using runType vs when would they use this? Basically outline when and why a client would want to implement this command.

@tgodzik
Copy link
Contributor Author

tgodzik commented Feb 13, 2023

Alright so just one more nit. Do you think we can also mention this in https://scalameta.org/metals/docs/integrations/debug-adapter-protocol and talk a bit about the intended flow. Mainly, in what situation would a client want to use the way that I questions about before using runType vs when would they use this? Basically outline when and why a client would want to implement this command.

Makes sense, I only mentioned shellCommand before, which is just very basic information.

@tgodzik
Copy link
Contributor Author

tgodzik commented Feb 13, 2023

Alright so just one more nit. Do you think we can also mention this in https://scalameta.org/metals/docs/integrations/debug-adapter-protocol and talk a bit about the intended flow. Mainly, in what situation would a client want to use the way that I questions about before using runType vs when would they use this? Basically outline when and why a client would want to implement this command.

I added some more explanation, let me know what you think! It's mostly an additional command that you can run from the client if you want to query metals. It's hopefully not very complicated.

@tgodzik tgodzik requested a review from ckipp01 February 13, 2023 18:08
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extra docs! LGTM!

@tgodzik tgodzik merged commit 254db45 into scalameta:main Feb 14, 2023
@tgodzik tgodzik deleted the add-run-command-discovery branch February 14, 2023 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants