-
Notifications
You must be signed in to change notification settings - Fork 328
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
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
6714899
to
027d30b
Compare
initialize
service for folders w/ non-Scala projectsa0f5b24
to
1ef05c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Some minor suggestions and questions.
metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/WorkspaceFolders.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/WorkspaceFolders.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/WorkspaceFolders.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/WorkspaceFolders.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
Outdated
Show resolved
Hide resolved
1ef05c4
to
6e56778
Compare
def isScalaDir(file: File): Boolean = { | ||
def isScalaProject(extendSearchToScriptsAndSbt: Boolean = true): Boolean = { | ||
val isScala: String => Boolean = | ||
if (extendSearchToScriptsAndSbt) _.isScalaFilename else _.isScala |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about Java source files, since Metals does provide some useful support for it? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, ideally you would start Java LSP server for those workspaces that are pure Java
I wonder about the interoperability with the |
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 |
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:
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 |
It would have to:
I also suppose it will be a very similar situation for Sublime. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (!isIn(services, folder)) { | ||
WorkspaceFoldersServices( | ||
services :+ newService, | ||
nonScalaProjects.filter(_ == folder), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nonScalaProjects.filter(_ == folder), | |
nonScalaProjects.filterNot(_ == folder), |
?
metals/src/main/scala/scala/meta/internal/metals/WorkspaceFolders.scala
Outdated
Show resolved
Hide resolved
@@ -163,8 +171,21 @@ class MetalsLanguageServer( | |||
val folderPaths = folders.map(_.path) | |||
|
|||
setupJna() | |||
|
|||
val folderPathsWithScala = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to use this in scribe.info
and for removing old reports below instead of folderPaths
?
Honestly I'm not sure right now. It was definitely mostly connected to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from me!
47d8dbc
to
1adb6b2
Compare
Previously: We had no distinction between workspace folders that are
Scala
projects and those that aren't.Now: We create
MetalsLspService
only forScala
projects.I consider something to be a Scala (metals) project if there is:
.metals
folder.scala
,.sc
,.sbt
) file.\src
,.\it
,.\test
,.\*\src
,.\*\it
,.\*\test
resolves: #5558