paulk-asert commented on PR #2976:
URL: https://github.com/apache/groovy/pull/2976#issuecomment-6392693261
Thanks Daniel. I checked the new head (5aa82c4422) against each item,
running the missing-jar cases for real rather than only through the tests.
Verified fixed:
- groovysh without groovy-lsp/LSP4J/Gson on the classpath: one warning, Tab
falls back to the shell completer, and the shell exits normally.
- noSyntaxCheck set in groovysh_init.groovy is now honoured. (Side effect
worth a line in groovysh.adoc: onReaderReady now runs after the init file
rather than before it.)
- Highlighting: the shift-by-delta plus the sorted merge is exactly what I
was after.
- @ASTTest: override && property is the right rule, and the THREAT_MODEL row
is updated.
- close() on the EDT returns right away, with the release done on the reaper
thread.
- All the should-fixes from my last comment.
One item left, the console half of the missing-jar fallback.
ConsoleTextEditor now falls back to GroovyFilter correctly, but
Console.installLanguageHost() (Console.groovy:625-633) has no guard, and
ConsoleLanguageHost's constructor calls GroovyLanguageClient.open. Compiler
Assistance is on by default, so with groovy-lsp absent, Console.main throws
NoClassDefFoundError right after the frame is shown. The script given on the
command line is never loaded, and the stack trace goes into the console's own
output pane (the interceptor is already installed), so the user just sees an
empty editor. The same unguarded call is reached from the new-window path
(:1168) and the Compiler Assistance toggle (:2364). The current tests simulate
only the editor-filter path.
Suggested fix: catch LinkageError in installLanguageHost, log the same
one-line warning, and leave languageHost null. Ideally also disable the
Compiler Assistance menu item, since it can't do anything in that case. Then
add a test that uses a throwing seam for attach, in the same way as the filter
test.
Two nits:
- ConsoleLanguageHost.groovy:195 has an orphaned Javadoc block ("Compiles
text immediately and returns diagnostics") with no method under it.
- ASTTestTransformation.currentThreadEnabled/setThreadEnabled are public,
since that's the Groovy default. As they're tooling-only, would @Internal be
worth adding?
With the installLanguageHost guard in, I'm happy for this to go into master
for 7.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
