Conversation
|
test this please |
| default void cleanScalaProjects(IProgressMonitor monitor) { | ||
| } |
There was a problem hiding this comment.
I'm not fond of this idea. This is a Buildship specific implementation detail that leaks to the API level. Could we avoid that?
There was a problem hiding this comment.
This isn’t related to Buildship. Gradle Tooling API expects Scala IDE to exist, adds SCALA_CONTAINER and removes some scala libraries.
Buildship will just take what Tooling API send to it. See
https://github.com/gradle/gradle/blob/b3c5d40e82439da4627b38b4ced93121e551b0eb/platforms/ide/ide-plugins/src/main/java/org/gradle/plugins/ide/eclipse/EclipsePlugin.java#L375-L377
Since Scala IDE doesn’t exist in VS Code, we need to add missing libraries and remove natures, builders and classpath containers that Java LS doesn’t recognize. If someone adds an extension that introduces the Scala Container, builder and nature, the cleanScalaProjects method won’t do anything.
There was a problem hiding this comment.
still, that is a very gradle-specific implementation detail. Why can't we call the cleanScalaProjects() implementation directly at the start of the GradleBuildSupport.compile(...) method?
There was a problem hiding this comment.
We do not call the cleanScalaProjects method whenever the compile method is called.
I removed this method from IBuildSupport.
| ProjectsManager projectsManager = JavaLanguageServerPlugin.getProjectsManager(); | ||
| if (projectsManager != null) { | ||
| projectsManager.buildSupports().forEach(bs -> { | ||
| bs.cleanScalaProjects(monitor); |
| */ | ||
| public class SearchUtils { | ||
|
|
||
| private static final class SearchClassVisitor extends ClassVisitor { |
There was a problem hiding this comment.
Could you document what this class does and why is there something scala-specific?
org.eclipse.jdt.ls.tests/projects/gradle/scala/app/build.gradle
Outdated
Show resolved
Hide resolved
| wp.refreshLocal(IResource.DEPTH_INFINITE, monitor); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Iterates through ALL workspace projects for every built project : O(n²) complexity for workspace with many projects 😱
Why is this necessary?
There was a problem hiding this comment.
If a gradle module has a kotlin, scala, aspectj or a groovy task, these tasks will be executed in all submodules.
If we don’t refresh submodules, JavaLS won’t create diagnostics correctly.
There was a problem hiding this comment.
shouldn't we at least filter only gradle projects?
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/GradleBuildSupport.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/GradleBuildSupport.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private void publishDiagnostics(String error, boolean parseKotlin, boolean parseGroovy, boolean parseAspectj) { | ||
| private void publishDiagnostics(String error, boolean parseKotlin, boolean parseGroovy, boolean parseAspectj, boolean parseScala) { |
There was a problem hiding this comment.
Can you wrap those booleans into a ParsingOptions record?
| return tempInit; | ||
| } | ||
|
|
||
| private static void process(IProject project, String output, IProgressMonitor monitor) { |
There was a problem hiding this comment.
Please add some documentation explaining what this large method does.
You can also try to break it down into several simpler pieces of code
| return null; | ||
| } | ||
|
|
||
| // Gradle Tooling API removes several scala libraries and adds the Scala builder and container that aren't recognized by Java LS. |
There was a problem hiding this comment.
maybe check if the plugin responsible for this Scala builder/container is present in this eclipse/jdt.ls instance, just in case we don't break someone already integrating scala with jdt.ls (unlikely but still)
There was a problem hiding this comment.
The cleanScalaProjects method checks all builders, natures, classpath containers and removes those that Java LS doesn’t recognize. We won’t break anyone. If Java LS recognizes a builder, nature or a classpath container, they won’t be removed.
|
@fbricon I have updated the PR. |
|
@fbricon I have updated the PR. |
2 participants
Fixes redhat-developer/vscode-java#4358
Requires redhat-developer/vscode-java#4359
cc @fbricon @rgrunber