[GitHub] [zookeeper] maoling commented on a change in pull request #1458: ZOOKEEPER-3938: java client: upgrade jline from 2.x to 3.x and adapt Completer to the new API.

[GitBox <gi...@apache.org>]

maoling commented on a change in pull request #1458:
URL: https://github.com/apache/zookeeper/pull/1458#discussion_r491420364



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/JLineZNodeCompleter.java
##########
@@ -43,40 +45,30 @@ public int complete(String buffer, int cursor, List candidates) {
         }
 
         if (token.startsWith("/")) {
-            return completeZNode(buffer, token, candidates);
-        }
-        return completeCommand(buffer, token, candidates);
-    }
-

Review comment:
       We can still use this skeleton to make the code more refined and less code changes?

##########
File path: pom.xml
##########
@@ -439,7 +439,7 @@
     <jetty.version>9.4.24.v20191120</jetty.version>
     <jackson.version>2.10.3</jackson.version>
     <json.version>1.1.1</json.version>
-    <jline.version>2.14.6</jline.version>
+    <jline.version>3.6.2</jline.version>

Review comment:
       Why choose the `3.6.2(Mar, 2018)`, instead [3.16.0(Jul, 2020)](https://mvnrepository.com/artifact/org.jline/jline/3.16.0)? 

##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeperMain.java
##########
@@ -283,44 +288,17 @@ void run() throws IOException, InterruptedException {
         if (cl.getCommand() == null) {
             System.out.println("Welcome to ZooKeeper!");
 
-            boolean jlinemissing = false;
-            // only use jline if it's in the classpath
-            try {
-                Class<?> consoleC = Class.forName("jline.console.ConsoleReader");
-                Class<?> completorC = Class.forName("org.apache.zookeeper.JLineZNodeCompleter");
-
-                System.out.println("JLine support is enabled");

Review comment:
       we still need to retain this line output?

##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeperMain.java
##########
@@ -283,44 +288,17 @@ void run() throws IOException, InterruptedException {
         if (cl.getCommand() == null) {
             System.out.println("Welcome to ZooKeeper!");
 
-            boolean jlinemissing = false;
-            // only use jline if it's in the classpath
-            try {
-                Class<?> consoleC = Class.forName("jline.console.ConsoleReader");
-                Class<?> completorC = Class.forName("org.apache.zookeeper.JLineZNodeCompleter");
-
-                System.out.println("JLine support is enabled");
+            Terminal terminal = TerminalBuilder.builder().system(true).build();
+            Completer znodeCompleter = new JLineZNodeCompleter(zk);
+            String prompt = "[zkCli] ";
 
-                Object console = consoleC.getConstructor().newInstance();
+            LineReader lineReader = LineReaderBuilder.builder().terminal(terminal).completer(znodeCompleter).build();

Review comment:
       Great, the new api looks more concise!




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org