improvement: don't create service for folders w/ non-Scala projects #5562

kasiaMarek · 2023-08-17T12:42:47Z

Previously: We had no distinction between workspace folders that are Scala projects and those that aren't.
Now: We create MetalsLspService only for Scala projects.

I consider something to be a Scala (metals) project if there is:

.metals folder
a toplevel scala+ (.scala, .sc, .sbt) file
a scala+ file in .\src, .\it , .\test, .\*\src, .\*\it , .\*\test
a scala+ file that belongs to that workspace folder was opened

resolves: #5558

tgodzik

Great work! Some minor suggestions and questions.

tests/unit/src/test/scala/tests/ReportsSuite.scala

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

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

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

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

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

filipwiech · 2023-08-29T14:48:59Z

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

-      def isScalaDir(file: File): Boolean = {
+    def isScalaProject(extendSearchToScriptsAndSbt: Boolean = true): Boolean = {
+      val isScala: String => Boolean =
+        if (extendSearchToScriptsAndSbt) _.isScalaFilename else _.isScala


What about Java source files, since Metals does provide some useful support for it? 🙂

Metals isn't really a Java LSP and for a pure Java project one should most probably use something else. However, you can always force metals (server) to acknowledge smth as a "metals project" by adding .metals dir.

Yeah, ideally you would start Java LSP server for those workspaces that are pure Java

filipwiech · 2023-08-29T14:53:44Z

I wonder about the interoperability with the nvim-metals plugin. I'm not sure about VS Code (I guess this PR introduces a similar mechanism?), but the Neovim plugin allows the user to configure which file types should cause Metals server to start and provide editor support. The default example configuration from the README defines scala, sbt and java files as triggers. Could there be a way to somehow unify those 2 approaches, so that they don't desynchronize depending on the user's configuration (vs hardcoded Metals settings)? Or maybe I misunderstood and there's no actual issue here. 🤔

kasiaMarek · 2023-08-29T15:11:44Z

I'm not sure about VS Code

It's just Scala

Could there be a way to somehow unify those 2 approaches, so that they don't desynchronize depending on the user's configuration (vs hardcoded Metals settings)?

We could add settings to have an array of possible triggers but I'm not sure it's worth it. The biggest use case seems to be java but it would be much simpler for the client (nvim-metals) to create a .metals directory in such case. Any thoughts @ckipp01 ?

ckipp01 · 2023-09-01T07:48:48Z

I'm not sure about VS Code

It's just Scala

Could there be a way to somehow unify those 2 approaches, so that they don't desynchronize depending on the user's configuration (vs hardcoded Metals settings)?

We could add settings to have an array of possible triggers but I'm not sure it's worth it. The biggest use case seems to be java but it would be much simpler for the client (nvim-metals) to create a .metals directory in such case. Any thoughts @ckipp01 ?

Good questions @filipwiech. nvim-metals works quite a bit different here as it relates to the actual issue this is trying to solve. I'm assuming that any "buffer-based" editor will probably behave the same. The idea of a "workspace" is sort of artificial in nvim-metals since it's always just a buffer you're opening. Therefore it always needs to detect if that file is actually a new workspace that is being opened or part of an existing one that was already sent in. So the original issue that relates to doesn't exist in nvim-metals unless you manually tried adding a workspace root that wasn't a Scala one.

In regards to Java, I'd actually consider what this PR does to be a regression. For myself personally I do actually use Metals sometimes on minimal Java projects that I don't want to spin up IntelliJ for. For me personally I don't do enough Java to justify the setup of the Java language server, which isn't easy in Neovim, and isn't a smooth experience. So what Metals offers me is just perfect for what I need. It'd be a shame if that was lost.

While I understand the original issue, I'm not fully sure I understand why it happens. Is this just VS Code sending in every root when you open up:

scala-folder
python-folder
typescript-folder

It sends in all three? Couldn't the VS Code extension have some sort of logic to only send in the correct workspace root and filter out the non-scala ones? I wouldn't expect python-folder and typescript-folder to be added as workspace roots. The extension should be able to determine that these aren't roots meant for Scala. I get that maybe Metals should guard a bit in the case that a user tries to send in a pointless directory as a root, but I don't think it should not allow a root (java) that it can totally work with.

kasiaMarek · 2023-09-01T09:31:17Z

Couldn't the VS Code extension have some sort of logic to only send in the correct workspace root and filter out the non-scala ones?

It would have to:

filter for the initial request,
filter for didChangeWorkspaceFolders ,
send additional notifications when a Scala file was created in a workspace folder, that previously was not considered a Scala project.

I also suppose it will be a very similar situation for Sublime.
However, it would be very easy to allow the client to opt out of this logic all together. We already have isKnownScalaProject flag.

tgodzik · 2023-09-05T13:19:34Z

Could we instead also detect Java files to satisfy the use case that @ckipp01 mentions? Ideally, we would not turn on if the java extension is present, but I am not sure how to check that properly

jkciesluk

Good job! About reports and limited files manager, this additional checks are needed because some error was occurring before? Or switching to folderPathsWithScala should solve the problem?
Although, I'm for having these checks either way.

jkciesluk · 2023-09-15T10:34:14Z

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

+        if (!isIn(services, folder)) {
+          WorkspaceFoldersServices(
+            services :+ newService,
+            nonScalaProjects.filter(_ == folder),


Suggested change

nonScalaProjects.filter(_ == folder),

nonScalaProjects.filterNot(_ == folder),

?

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

jkciesluk · 2023-09-15T11:06:17Z

metals/src/main/scala/scala/meta/metals/MetalsLanguageServer.scala

@@ -163,8 +171,21 @@ class MetalsLanguageServer(
          val folderPaths = folders.map(_.path)

          setupJna()
+
+          val folderPathsWithScala =


Don't we want to use this in scribe.info and for removing old reports below instead of folderPaths?

kasiaMarek · 2023-09-18T08:30:25Z

some error was occurring before? Or switching to folderPathsWithScala should solve the problem?

Honestly I'm not sure right now. It was definitely mostly connected to folderPaths instead of folderPathsWithScala. But maybe is better anyway not to create all those dirs eagerly.

tgodzik

LGTM from me!

kasiaMarek force-pushed the handle-non-scala-projects branch from 6714899 to 027d30b Compare August 18, 2023 16:04

kasiaMarek changed the title ~~improvement: don't set up logger or initialize service for folders w/ non-Scala projects~~ improvement: don't create service for folders w/ non-Scala projects Aug 18, 2023

kasiaMarek marked this pull request as draft August 18, 2023 16:17

kasiaMarek force-pushed the handle-non-scala-projects branch 4 times, most recently from a0f5b24 to 1ef05c4 Compare August 22, 2023 14:48

kasiaMarek marked this pull request as ready for review August 23, 2023 09:00

kasiaMarek requested review from tgodzik and jkciesluk August 23, 2023 09:00

tgodzik requested changes Aug 29, 2023

View reviewed changes

kasiaMarek force-pushed the handle-non-scala-projects branch from 1ef05c4 to 6e56778 Compare August 29, 2023 14:35

filipwiech reviewed Aug 29, 2023

View reviewed changes

kasiaMarek requested a review from tgodzik September 7, 2023 16:31

jkciesluk requested changes Sep 15, 2023

View reviewed changes

kasiaMarek requested a review from jkciesluk September 21, 2023 14:00

tgodzik approved these changes Sep 26, 2023

View reviewed changes

kasiaMarek added 3 commits September 27, 2023 14:53

improvement: do not create MetalsLspService for non-Scala projects

7659e3a

start MetalsLspService also for java projects

b25efa3

review fixes

1adb6b2

kasiaMarek force-pushed the handle-non-scala-projects branch from 47d8dbc to 1adb6b2 Compare September 27, 2023 12:58

jkciesluk approved these changes Oct 2, 2023

View reviewed changes

kasiaMarek merged commit c85e193 into scalameta:main Oct 2, 2023
22 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement: don't create service for folders w/ non-Scala projects #5562

improvement: don't create service for folders w/ non-Scala projects #5562

kasiaMarek commented Aug 17, 2023 •

edited

tgodzik left a comment

filipwiech Aug 29, 2023

kasiaMarek Aug 29, 2023

tgodzik Aug 29, 2023

filipwiech commented Aug 29, 2023 •

edited

kasiaMarek commented Aug 29, 2023

ckipp01 commented Sep 1, 2023

kasiaMarek commented Sep 1, 2023

tgodzik commented Sep 5, 2023

jkciesluk left a comment

jkciesluk Sep 15, 2023

jkciesluk Sep 15, 2023

kasiaMarek commented Sep 18, 2023

tgodzik left a comment

	nonScalaProjects.filter(_ == folder),
	nonScalaProjects.filterNot(_ == folder),

improvement: don't create service for folders w/ non-Scala projects #5562

improvement: don't create service for folders w/ non-Scala projects #5562

Conversation

kasiaMarek commented Aug 17, 2023 • edited

tgodzik left a comment

Choose a reason for hiding this comment

filipwiech Aug 29, 2023

Choose a reason for hiding this comment

kasiaMarek Aug 29, 2023

Choose a reason for hiding this comment

tgodzik Aug 29, 2023

Choose a reason for hiding this comment

filipwiech commented Aug 29, 2023 • edited

kasiaMarek commented Aug 29, 2023

ckipp01 commented Sep 1, 2023

kasiaMarek commented Sep 1, 2023

tgodzik commented Sep 5, 2023

jkciesluk left a comment

Choose a reason for hiding this comment

jkciesluk Sep 15, 2023

Choose a reason for hiding this comment

jkciesluk Sep 15, 2023

Choose a reason for hiding this comment

kasiaMarek commented Sep 18, 2023

tgodzik left a comment

Choose a reason for hiding this comment

kasiaMarek commented Aug 17, 2023 •

edited

filipwiech commented Aug 29, 2023 •

edited