-
Notifications
You must be signed in to change notification settings - Fork 154
[TRAFODION-2211] trafci can use history cmd #879
Conversation
Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1465/ |
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1465/ |
8618a51
to
a88b4dc
Compare
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1468/ |
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1468/ |
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.
This is a super cool new feature. I have a question though, need your clarification.
@@ -172,7 +176,8 @@ public String readLine() throws UnsupportedEncodingException, UserInterruption, | |||
|
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.
This will remove Ctrl-C signal handler, did you test Ctrl-C in trafci? It should do a 'cancel query' if I remember it correctly. Anyway, you should make sure Ctrl-C has same behavior as before, should not be broken by this change.
I am not sure, need to check the trafci manual, but wondering if you have tested it or not? Since you comments out the signal handler.
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.
ohh, i really not notice that.. i will research how to add this behavior.
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1472/ |
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1472/ |
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.
Looks good to me now. Very thankful to have this feature added, this is a very user friendly feature.
+1
|
||
@@@ END COPYRIGHT @@@ | ||
--> | ||
<!-- @@@ START COPYRIGHT @@@ Licensed to the Apache Software Foundation (ASF) |
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.
Not sure what happened to the license text here. Could we put the original back?
|
||
@@@ END COPYRIGHT @@@ | ||
--> | ||
<!-- @@@ START COPYRIGHT @@@ Licensed to the Apache Software Foundation (ASF) |
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.
Same comment about the license text.
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.
This is mostly outside my area of expertise. The license text seems to have been changed unnecessarily in two files.
This change is definitely very helpful. Is the change in java/org/trafodion/ci/ConsoleReader.java alone sufficient to take care of history cmd? If so, can you please add some description for other changes. |
the most important change is in java/org/trafodion/ci/ConsoleReader.java, and the change in java/org/trafodion/ci/Session.java is help to remove duplicates the prompt. |
pom.xml & installer_pom.xml : add dependence of jline and format it(replace tab to 4 blanks). |
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1480/ |
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1480/ |
Thanks @mashengchen for your explanation. Please consider adding all other issues fixed in the JIRA too. +1 Looks good |
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.
+1 Thanks for reformatting the licence texts. Looks like this is ready to merge to me.
trafci should support up, down, left, right key in the keyboard