Display build target info #3380

Arthurm1 · 2021-12-17T14:24:46Z

This is an alternative to #3077 which I think I'll ditch in favour of this.

Instead of webview it uses metalsdecode: and virtual docs. The benefits are that it should be able to be supported in the same way across different clients (vscode, vim, sublime). It also means that multiple build target views can be open at once. The downside is that it loses the navigation to other build targets and to jars.

You can open a uri with the format metalsDecode:file:///workspacePath/buildTargetName.metals-buildtarget in the same way as other metalsDecode types work and Metals will hand you back the text describing that build target.
You can fire off target-info-display command and Metals will use the QuickPick functionality to offer a list of target names.

There's a textmate grammar file for highlighting the build target file.

@ckipp01 @ayoub-benali I've kind of assumed that since you have the metalsDecode functionality in Vim/Sublime that this will be easy to add?

This is rebased off the Java PR #2520 as it sets up all the javacoptions so there's no point reviewing til that's merged.

metals-1639750923059.mp4

GavinRay97 · 2021-12-20T16:23:14Z

This is really cool + useful, thank you 👀

ayoub-benali · 2021-12-20T17:38:41Z

Cool feature 🎉 and yes I shouldn't be too difficult to add

ckipp01 · 2021-12-21T09:52:32Z

Same from me on the nvim-metals side. This should be easy to add 👍🏼

tgodzik

Great work! Finally I will not have to go manually into .bloop files 😅

And probably this would be much easier for sbt server than before. Left some questions and we should probably rebase.

tgodzik · 2022-01-31T10:37:27Z

metals/src/main/scala/scala/meta/internal/metals/BuildTargetInfo.scala

+class BuildTargetInfo(buildTargets: BuildTargets) {
+
+  def buildTargetDetails(targetName: String): String = {
+    buildTargets.allCommon


Looks like we need to rebase on recent main? We renamed allCommon from what I remember.

tgodzik · 2022-01-31T10:39:50Z

metals/src/main/scala/scala/meta/internal/metals/BuildTargetInfo.scala

+        val padding = " " * (maxFilenameSize - filename.size)
+        val status = if (f.toFile.exists) {
+          if (f.toFile().isDirectory())
+            "         "


Could we make it a value? The same length is used underneath I think? And maybe do soemthing like " " * 5 ?

tgodzik · 2022-01-31T12:39:12Z

metals/src/main/scala/scala/meta/internal/tvp/ClasspathTreeView.scala

@@ -33,7 +34,8 @@ class ClasspathTreeView[Value, Key](
      viewId,
      rootUri,
      title + s" (${toplevels().size})",
-      collapseState = MetalsTreeItemCollapseState.collapsed
+      collapseState = MetalsTreeItemCollapseState.collapsed,
+      contextValue = "root"


Why do we need the new contextValue ? This should be documented in tree view protocol https://scalameta.org/metals/docs/integrations/tree-view-protocol if we add it here.

It's so VSCode can show a context menu only on projects, not libraries, in the treeview. If you're ok with that then I'll add some docs

Would we be able to do it in VS Code only? I would rather avoid changing it if it's not needed for any other extensions. @ckipp01 what do you think?

Or maybe, we could make it general and add command filed instead something like:

"comand" : { "id" : "show-target", args: [] }

what do you think?

The Display Build Target command works from VSCode. This was just an extra way of doing it - i.e. right click on a project in the Metals TreeView and get the info. I'm cool with removing that if you'd prefer not to change the treeview API

I think that is a cool option to have it under right click, just wondering if we can make the API change less VS Code reliant. I am also worried that with this change we will need to update all the clients.

Actually, let me take a look if we can do it without the added parameter

If at all possible I think we should avoid baking in VS Code specific stuff to the actual protocol itself. The more we do that the harder it becomes to work around for all other clients. I don't really understand the full context behind this to be honest. I don't know what a context menu is.

ContextValue is gone. The context menu allows you to right-click on a project in the treeview and get a menu up to allow you to call Display the Build Target info directly on the project/target, instead of running the Display Build Target command and getting a list of project/targets up. Just a different way of getting to view project info

ckipp01 · 2022-01-31T12:48:31Z

@ckipp01 @ayoub-benali I've kind of assumed that since you have the metalsDecode functionality in Vim/Sublime that this will be easy to add?

Yup, the handling of just the raw text would be fine. Although I honestly think this would be a killer feature to return to the editor in a structured manner instead of just text. I know for myself this was actually what I proposed in #2479 but kind of got shot down. If the client can have access to this info, it unlocks a ton of stuff they can do with it. So the text is great, but I'd honestly love this as JSON.

ckipp01

I think we have a lot of duplication in here with already existing stuff that things like BuildTarget already gives is. Is it possible to maybe slim down all the extra classes that are added here and to instead use the existing ones?

ckipp01 · 2022-01-31T12:58:30Z

metals/src/main/scala/scala/meta/internal/metals/BuildTargetInfo.scala

+      targetId: BuildTargetIdentifier
+  ): Option[String] = {
+    extractTargetDetail(targetId).map(detail => {
+      val info = ListBuffer[String]()


Sort of a nit, but you could also just use a string builder here.

With StringBuilder I think I'd need to add newlines everywhere

ckipp01 · 2022-01-31T12:59:03Z

metals/src/main/scala/scala/meta/internal/metals/BuildTargetInfo.scala

+      if (detail.common.tags.nonEmpty) {
+        info += ""
+        info += "Tags"
+        detail.common.tags.foreach(f => info += s"  $f")


Can we replace all the usages of f with more meaning names.

ckipp01 · 2022-01-31T13:01:50Z

metals/src/main/scala/scala/meta/internal/metals/BuildTargetInfo.scala

+      languageIds: List[String],
+      dependencies: List[String],
+      dependentTargets: List[String],
+      capabilities: List[(String, Boolean)],


It'd be nicer if this could be a List[BuildTargetCapability]. Then we could also avoid the ._2 shenanigans up above.

Actually thinking about this, there seems to be a lot of redundancy in here. Are all of these classes actually needed. For example we could also just use BuildTargetCapabilities from BSP for this couldn't we? Same with almost all of this stuff in here. This is very close to the actual BuildTarget interface.

ckipp01 · 2022-01-31T13:04:12Z

metals/src/main/scala/scala/meta/internal/metals/BuildTargetInfo.scala

+      else
+        path
+    }
+    if (classPath.nonEmpty) {


I'm recommend breaking this up a bit. We have 3 nested ifs in here that can be tricky to follow. Breaking those out to functions might make this easier to read.

ckipp01 · 2022-01-31T13:07:30Z

metals/src/main/scala/scala/meta/internal/metals/BuildTargetInfo.scala

+    var capabilities = List.empty[(String, Boolean)]
+    capabilities ::= ("Debug", target.capabilities.getCanDebug)
+    capabilities ::= ("Test", target.capabilities.getCanTest)
+    capabilities ::= ("Run", target.capabilities.getCanRun)
+    capabilities ::= ("Compile", target.capabilities.getCanCompile)
+    capabilities


Lets avoid the var here. We can just create a list and add these when we create the list like one :: two :: three :: nil and just return that. Although again as I stated above, we should avoid a tuple here, and even avoid the extra class if we can.

Arthurm1 · 2022-01-31T20:48:20Z

@ckipp01 @ayoub-benali I've kind of assumed that since you have the metalsDecode functionality in Vim/Sublime that this will be easy to add?

Yup, the handling of just the raw text would be fine. Although I honestly think this would be a killer feature to return to the editor in a structured manner instead of just text. I know for myself this was actually what I proposed in #2479 but kind of got shot down. If the client can have access to this info, it unlocks a ton of stuff they can do with it. So the text is great, but I'd honestly love this as JSON.

From what I can see you want the client to be able to interact with the build server e.g. query targets or run a mainclass?

Instead of creating multiple commands for whatever the client wants from the underlying build server, how about creating a generic pass this command to BSP command and get Metals to send back the BSP json results. Then the client can call whatever BSP stuff it wants without having to special case each requirement in Metals.

Arthurm1 · 2022-01-31T20:49:55Z

@tgodzik I don't understand the ScalaFix error here. Can you help?

tgodzik · 2022-02-01T12:18:13Z

@tgodzik I don't understand the ScalaFix error here. Can you help?

It doesn't seem related. I will rerun.

Arthurm1 · 2022-02-01T15:24:33Z

@tgodzik InsertInferredTypeLspSuite.match-bind fails sometimes but has nothing to do with this.

It seems to fail because in inferTypeTitle a different match occurs...

Here's the code snippet...

    pattern.parent
          .collect {
            case Pat.Typed(_, _) => None
            case Pat.Bind(lhs, _) if lhs != pattern => Some(insertTypeToPattern)
            case _: Pat.Bind => None
            case vl: Defn.Val if vl.decltpe.isEmpty => Some(insertType)
            case vr: Defn.Var if vr.decltpe.isEmpty => Some(insertType)
            case _: Pat | _: Enumerator => Some(insertTypeToPattern)
          }
          .getOrElse(None)

Sometimes the test enters...
case Pat.Bind(lhs, _) if lhs != pattern => Some(insertTypeToPattern)
Sometimes it enters...
case _: Pat.Bind => None

One passes, the other fails. I don't know why this randomly happens as I have no idea what this code is supposed to do. It's fairly easy to reproduce the failure when you run the whole
InsertInferredTypeLspSuite suite but if you just run InsertInferredTypeLspSuite-match-bind it always passes.

tgodzik · 2022-02-01T15:34:48Z

@tgodzik InsertInferredTypeLspSuite.match-bind fails sometimes but has nothing to do with this.

It seems to fail because in inferTypeTitle a different match occurs...

Sometimes the test enters... case Pat.Bind(lhs, _) if lhs != pattern => Some(insertTypeToPattern) Sometimes it enters... case _: Pat.Bind => None

One passes, the other fails. I don't know why this randomly happens as I have no idea what this code is supposed to do. It's fairly easy to reproduce the failure when you run the whole InsertInferredTypeLspSuite suite but if you just run InsertInferredTypeLspSuite-match-bind it always passes.

Och, I've seen this happen before, but couldn't reproduce it.

tgodzik · 2022-02-01T15:35:18Z

I will take a look!

Arthurm1 · 2022-02-04T20:21:15Z

@tgodzik I think this is ready to merge.

tgodzik

LGTM!

Arthurm1 force-pushed the virtual_display_build_target branch from 80ffbd8 to 1eda1e2 Compare December 21, 2021 12:29

This was referenced Dec 21, 2021

Display build target info #3077

Closed

display build target info scalameta/metals-vscode#660

Merged

ayoub-benali mentioned this pull request Dec 23, 2021

Add target-info-display command scalameta/metals-sublime#78

Closed

Arthurm1 force-pushed the virtual_display_build_target branch from 1eda1e2 to c181998 Compare December 25, 2021 14:34

Arthurm1 marked this pull request as ready for review December 28, 2021 15:51

tgodzik requested changes Jan 31, 2022

View reviewed changes

ckipp01 reviewed Jan 31, 2022

View reviewed changes

tgodzik added the affects clients Use this if you are adding a new setting or making a change that will affect clients. label Jan 31, 2022

Arthurm1 force-pushed the virtual_display_build_target branch 2 times, most recently from f40ec08 to 41d2438 Compare January 31, 2022 19:36

Arthurm1 added 2 commits February 4, 2022 15:46

Display build target info

b3b8b4b

Address Build Target feedback

b5566cf

Arthurm1 force-pushed the virtual_display_build_target branch from 41d2438 to b5566cf Compare February 4, 2022 15:47

tgodzik approved these changes Feb 7, 2022

View reviewed changes

tgodzik merged commit c86af49 into scalameta:main Feb 7, 2022

ckipp01 mentioned this pull request Feb 7, 2022

Add support for the build target info scalameta/nvim-metals#339

Closed

Arthurm1 deleted the virtual_display_build_target branch February 7, 2022 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display build target info #3380

Display build target info #3380

Arthurm1 commented Dec 17, 2021

GavinRay97 commented Dec 20, 2021

ayoub-benali commented Dec 20, 2021

ckipp01 commented Dec 21, 2021

tgodzik left a comment

tgodzik Jan 31, 2022

tgodzik Jan 31, 2022

tgodzik Jan 31, 2022

Arthurm1 Jan 31, 2022

tgodzik Jan 31, 2022

Arthurm1 Jan 31, 2022

tgodzik Jan 31, 2022

tgodzik Jan 31, 2022

ckipp01 Jan 31, 2022

Arthurm1 Jan 31, 2022

ckipp01 commented Jan 31, 2022

ckipp01 left a comment

ckipp01 Jan 31, 2022

Arthurm1 Jan 31, 2022

ckipp01 Jan 31, 2022

ckipp01 Jan 31, 2022

ckipp01 Jan 31, 2022

ckipp01 Jan 31, 2022

ckipp01 Jan 31, 2022

Arthurm1 commented Jan 31, 2022

Arthurm1 commented Jan 31, 2022

tgodzik commented Feb 1, 2022

Arthurm1 commented Feb 1, 2022

tgodzik commented Feb 1, 2022

tgodzik commented Feb 1, 2022

Arthurm1 commented Feb 4, 2022

tgodzik left a comment

Display build target info #3380

Display build target info #3380

Conversation

Arthurm1 commented Dec 17, 2021

GavinRay97 commented Dec 20, 2021

ayoub-benali commented Dec 20, 2021

ckipp01 commented Dec 21, 2021

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckipp01 commented Jan 31, 2022

ckipp01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arthurm1 commented Jan 31, 2022

Arthurm1 commented Jan 31, 2022

tgodzik commented Feb 1, 2022

Arthurm1 commented Feb 1, 2022

tgodzik commented Feb 1, 2022

tgodzik commented Feb 1, 2022

Arthurm1 commented Feb 4, 2022

tgodzik left a comment

Choose a reason for hiding this comment