Add extra options when launching Java langserver if JDK's version > 8 #36100
Conversation
💔 Build Failed |
| if (javaVersion == 8) { | ||
| this.needExtraOptions = false; | ||
| } | ||
| if (javaVersion >= 8) { |
There was a problem hiding this comment.
do you mean <= 8, we do false, and > 8 we do true?
There was a problem hiding this comment.
then shouldn't we do?
if (javaVersion <= 8) {
this.needModuleArguments = false;
}
if (javaVersion > 8) {
``
?
There was a problem hiding this comment.
Maybe the semantic here is not very clear. If java version < 8, we use the bundled JDK(whose version >8), so we need extra arguments by default. When system jdk's version >= 8, we use the system JDK, while only for jdk's version = 8, we needn't extra arguments.
There was a problem hiding this comment.
ok, maybe use <= instead of ==, although it cover more case that not existed, yet it's easier to understand.
or you could put those comments there so people don't get confuse
| @@ -19,6 +19,7 @@ import { RequestExpander } from './request_expander'; | |||
|
|
|||
| export class JavaLauncher implements ILanguageServerLauncher { | |||
| private isRunning: boolean = false; | |||
| private needExtraOptions: boolean = true; | |||
There was a problem hiding this comment.
maybe rename to needModuleArguments
| if (javaVersion == 8) { | ||
| this.needExtraOptions = false; | ||
| } | ||
| if (javaVersion >= 8) { |
There was a problem hiding this comment.
then shouldn't we do?
if (javaVersion <= 8) {
this.needModuleArguments = false;
}
if (javaVersion > 8) {
``
?
💔 Build Failed |
💔 Build Failed |
|
Reverted in 7a4a884 Please don't merge PRs with failing builds |
Summary
Resolve: https://github.com/elastic/code/issues/1168