-
Notifications
You must be signed in to change notification settings - Fork 333
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
implement Java folding using JDK scanner #5328
Conversation
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! Sorry for not getting to it sonner!
import org.eclipse.lsp4j.FoldingRange | ||
import org.eclipse.lsp4j.FoldingRangeKind | ||
|
||
final class JavaFoldingRangeExtractor( | ||
text: String, | ||
foldOnlyLines: Boolean, |
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.
foldOnlyLines
is used when you can't fold a part of the line. So potentially we should always encapsulated to whole lines in the tests, not sure if it's currently done for Scala, we might just have a couple of unit tests.
metals/src/main/scala/scala/meta/internal/parsing/JavaFoldingRangeExtractor.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/parsing/JavaFoldingRangeExtractor.scala
Show resolved
Hide resolved
def extract( | ||
text: String, | ||
path: AbsolutePath, | ||
// TODO - what is this supposed to do??? Is it relevant in Java? |
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.
As mentioned above foldOnlyLines is used when you can't fold a part of the line.
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.
I've added code to handle it - if I understand it correctly
metals/src/main/scala/scala/meta/internal/parsing/JavaFoldingRangeExtractor.scala
Outdated
Show resolved
Hide resolved
@tgodzik I think everything's resolved - can we merge? |
|
||
import org.eclipse.lsp4j.FoldingRange; | ||
import org.eclipse.lsp4j.FoldingRangeKind;<<imports<< | ||
>>imports>>import java.io.File; |
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.
Last thing, could add the test to foldingRange-scala3-foldLineOnly
folder also? It should handle Java properly (though it would probably better to create a separate suite, but let's not do it now.) This way we'll see the difference between fold only on and off
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.
Done
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! Thanks for working on this!
This shifts the Java folding from using the Eclipse JDT to using the JDK.
The issue with the latest Eclipse JDT is that it requires Java 11 to run, so Metals has to use an old version of Eclipse and thus can't support newer Java language features e.g. triple-quote Strings.
The code here uses the Java
TreePathScanner
to identify code blocks and Strings but it ignores comments so searching for them is done separately.I'm not sure what
foldOnlyLines
was supposed to do - I've ignored it for now.I've used the code in
JavaMetalsGlobal
to analyse the Java source. I imagine hover, completions, folding all use this in the same way so eventually maybe the Java equivalent ofscala.meta.internal.parsing.Trees
could be used for caching these results.