-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add autocomplete based on omelette library #907
Conversation
index.js
Outdated
@@ -452,6 +686,9 @@ Command.prototype.allowUnknownOption = function(arg) { | |||
*/ | |||
|
|||
Command.prototype.parse = function(argv) { | |||
// trigger autocomplete first | |||
this.autocomplete(argv); |
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.
parse will always call autocomplete transparently, in this way autocomplete method don't have to be a public user API.
For normal cases, overhead should be acceptable since we only have several string comparison operations.
If argv shows that this request is likely for autocomplete, omelette will take over and program.exit(0) after processing.
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.
Maybe we should better hide auto-completion feature behind the flag and set it to false
by default?
@@ -24,7 +24,9 @@ | |||
"index.js", | |||
"typings/index.d.ts" | |||
], | |||
"dependencies": {}, | |||
"dependencies": { | |||
"omelette": "^0.4.12" |
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.
added production dependency to omelette, which target supports for node 5+, I guess it should be ok?
@abetomo Would you mind help review this PR? Thank you so much. |
@vanesyan Would you mind help look into this PR when you're available? |
@abetomo sorry to bother, but we haven't yet heard back from @vanesyan for a while, can you help somehow reach out to him? |
index.js
Outdated
@@ -452,6 +686,9 @@ Command.prototype.allowUnknownOption = function(arg) { | |||
*/ | |||
|
|||
Command.prototype.parse = function(argv) { | |||
// trigger autocomplete first | |||
this.autocomplete(argv); |
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.
Maybe we should better hide auto-completion feature behind the flag and set it to false
by default?
Hello there, sorry for me make you waiting so long. I've partially reviewed the per, average PR looks good, I'll try to cover another part of PR in review. Also I worry about breaking changes, as now we reserve some flags which may be used in userland code, so maybe we can hide behind a flag in commander API. |
Thanks @vanesyan for the careful review. I'll address the styling issues soon. For the breaking changes of autocompletion flag, how about if we add a guardian condition that only if we notice at least one autocompletion rules are registered, we trigger autocompletion logic, otherwise we simply skip this function so even if previously user had defined options in those reserved keywords commander will still work as expected? |
@abetomo What do you thing about this change? Should we hide it explicitly behind the flag, or enable it once user defines rules for autocomplete? |
There is a lot of interesting work in this Pull Request, and credit for including documentation and tests. But I want to raise a concern that might have been missed. Commander currently has zero production dependencies. Although omelette is described as a "lean library for autocompletion that has no further dependencies", it is currently 20x larger than commander.
I dug a little deeper and found Omelette package includes everything including images for README, and could be much leaner. I found an open issue along these lines: (I have yet to try the code in this Pull Release. This comment based on inspection of the Pull Request.) |
Thanks for pointing out the size problem. I have created an PR f/omelette#36 to shrink the size of upstream omelette package, with the new version the size (24.2kB) issue should be less concerned. |
There is a lot of good work in this pull request, but I have concerns that this adds maintenance complexity and code and a dependency for a feature that only some people will benefit from. I do appreciate that some people writing a CLI program would like completion. I am going to take a closer look, but letting you know I may recommend no. In which case, I wonder if an alternative is an add-on package which we can reference from the README rather than built-in. (One of the first packages I came across when I first looked a couple of years ago was the defunct commander-tabtab ). |
I will say again, this is a very well written Pull Request! And thanks for addressing some of the concerns, including submitting a Pull Request in Omelette (not yet released). However, this is not something we want to add into Commander itself at this time, due to the added complexity and maintenance we would be taking on, including reliance on a third party. If you decide to publish this in some form yourself, please let us know. Thank you for your contributions. |
I've used commander.js for a lot of interesting projects now. It's my honor to help contribute something back to Commander. I'll look into some other ways of bringing the autocomplete capability to the community. |
i would sugguest commander to allow plugin extensions and this would surely be great to be a plugin 👍 |
@bencao Did you have any luck extracting this PR into an extension / module? |
For anyone seeking a solution, I’ve developed @gutenye/commander-completion-carapace using Carapace. It's fast, works well, and supports numerous shells like Bash, Zsh, Fish, Nushell, and others. |
Add built-in autocomplete for commander.js.
Interface Design
The current proposed developer API looks like this
single command mode
sub command mode
And end users need to manually enable autocompletion by running
Implementation
Under the hood, we added a new production dependency to omelette, which is a lean library for autocompletion that has no further dependencies.
What we basically do is by mapping user-defined autocompletion rules into a form that is supported by omelette. We utilized the "Global complete Event" from omelette and should handle all completion cases gracefully:
closes #385.
Test and document also updated accordingly.
Please let me know if any more work should be included for this MR ❤️ .