-
Notifications
You must be signed in to change notification settings - Fork 916
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
ARTEMIS-4372 Implement Pico-cli and script auto-complete #4565
Conversation
ba030cb
to
35e9c6c
Compare
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/AutoCompletion.java
Outdated
Show resolved
Hide resolved
4e47aee
to
d032d26
Compare
dcde5cb
to
469bf63
Compare
I think the ability to set and clear the URI + credentials used for subsequent commands in the shell is good. I'm less clear on it doing it silently in the background based on every previous command. At least one of the additions you made was around it handling the URIs crossing between the different clients, presumably as the URI is saved but the protocol value is not, probably how you hit that, the client used will change between commands in some cases, whilst the URI + credentials etc do not even though the URIs are client-specific. |
5f93a97
to
0ab4a7f
Compare
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/Artemis.java
Outdated
Show resolved
Hide resolved
for my future reference. I opened an issue and Pull Request on Pico: remkop/picocli#2075 the jshell auto completion is showing hidden values. the PR sent would fix it but I'm waiting for a best way to merge this upstream in Pico. hidden is the way to go to "deprecate" stuff. |
eb5a4ae
to
d3da085
Compare
Only thing left on my todo is the JDK parameters and testing with a newer version. will do it before the end of today. |
c525d25
to
1e4e8fc
Compare
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.
You need to add jansi library, which is an optional dependency of JLine for better support.
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/Shell.java
Outdated
Show resolved
Hide resolved
artemis-cli/src/main/resources/org/apache/activemq/artemis/cli/commands/bin/artemis
Outdated
Show resolved
Hide resolved
--add-opens java.base/java.lang=ALL-UNNAMED \ | ||
--add-opens java.base/java.io=ALL-UNNAMED \ | ||
--add-opens java.base/java.util=ALL-UNNAMED \ |
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.
See above, adding jansi will remove the need for those options.
bb2a7e1
to
7a63e40
Compare
I did the change manually.. thank you so much
36910d2
to
7a992ae
Compare
ARTEMIS-4375 Implement artemis shell using JLine3 integrated with auto-completion from picocli This commit involves two JIRAs. One is adding PicoCLI and the next is Using JLine3 and implement a shell. I have tried to keep these commits separate but these changes became interdependent hence the two JIRAs are squashed in this commit.
@gemmellr I am merging this... further changes can be sent into main after this. Thanks for your help on this |
} catch (Exception e2) { | ||
} |
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 one does nothing, the earlier ones for Qpid JMS print the stack. Seems like they should be the same one way or the other.
} catch (Exception e2) { | ||
} |
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.
Ditto
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 really in both cases.. This is the exception of the exception. IN case the input user/password does not succeed, if this does not work this will simply not save the password here.
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 see what you mean now.
context.out.println("--txt-size is deprecated, please use --commit-interval"); | ||
txBatchSize = oldBatchSize; |
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.
If both were somehow set, I would expect the new one to always win...here the old one will win.
if (txSize > 0) { | ||
System.out.println("--tx-size is deprecated, please use --commit-interval"); | ||
commitInterval = txSize; | ||
} |
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.
If both were somehow set, I would expect the new one to always win...here the old one will win.
|
||
// Pico shouldn't allow generating a commandLine without an userObject. | ||
// the following assert "should" never happen | ||
assert userObject != null; |
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.
Good chance the assert also wont happen unless people turn on assertions. Seems like a null check would have been simpler. Though it will at least still NPE later, when failing to throw the IAE.
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 never supposed to happen.
Only chance userObject is null is if you set the userObject wrongly during development. and picocli actually validate that already.
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.
Fair enough. You didnt note the last bit before, just that 'we shouldnt add things that arent Runnable or Action', then you added the fallback check which would NPE if it was null, which I then noted. I'd still have just added a simple Objects.nonNull("oh-dear developer-error", userObject) rather than the multi line assert+comment that quite posibly still wont do anything and allow the raw NPE to occur.
No description provided.