Skip to content
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

Closed
wants to merge 9 commits into from

Conversation

bencao
Copy link

@bencao bencao commented Jan 17, 2019

Add built-in autocomplete for commander.js.

Interface Design

The current proposed developer API looks like this

single command mode

program
  .arguments('<a> <b>')
  .option('--verbose', 'verbose')
  .option('-n, --name <name>', 'specify name')
  .option('--description <desc>', 'specify description')
  .complete({
    options: {
      '--name': function() { return ['kate', 'jim']; },
      '--description': ['desc1', 'desc2']
    },
    arguments: {
      a: function() { return ['a-1', 'a-2']; },
      b: ['b-1', 'b-2']
    }
  })

sub command mode

program
  .command('sub1 <arg1> <arg2>')
  .description('sub command 1')
  .option('--verbose', 'verbose')
  .option('-n, --name <name>', 'specify name')
  .option('--description <desc>', 'specify description')
  .complete({
    options: {
      '--name': function() { return ['kate', 'jim']; },
      '--description': ['d1', 'd2']
    },
    arguments: {
      arg1: function() { return ['a-1', 'a-2', 'a-3']; },
      arg2: ['b-1', 'b-2']
    }
  });

program
  .command('sub2 <arg1>')
  .description('sub command 2')
  .option('--verbose', 'verbose')
  .option('-n, --name <name>', 'specify name')
  .option('--description <desc>', 'specify description')
  .complete({
    options: {
      '--name': function() { return ['lucy', 'linda']; },
      '--description': ['db1', 'db2']
    },
    arguments: {
      arg1: function() { return ['a-11', 'a-12']; },
    }
  });

And end users need to manually enable autocompletion by running

eval "$(command-name --completion)"

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:

var completion = omelette('git');

completion.on('complete', function(f, event) {
  command.autocompleteHandleEvent(event);
});

// omelette will call process.exit(0) after processing
completion.init();

closes #385.

Test and document also updated accordingly.

Please let me know if any more work should be included for this MR ❤️ .

index.js Outdated
@@ -452,6 +686,9 @@ Command.prototype.allowUnknownOption = function(arg) {
*/

Command.prototype.parse = function(argv) {
// trigger autocomplete first
this.autocomplete(argv);
Copy link
Author

@bencao bencao Jan 22, 2019

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.

Copy link
Collaborator

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"
Copy link
Author

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?

@bencao
Copy link
Author

bencao commented Jan 22, 2019

@abetomo Would you mind help review this PR? Thank you so much.

@bencao
Copy link
Author

bencao commented Feb 6, 2019

@vanesyan Would you mind help look into this PR when you're available?

@bencao
Copy link
Author

bencao commented Feb 13, 2019

@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 Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated
@@ -452,6 +686,9 @@ Command.prototype.allowUnknownOption = function(arg) {
*/

Command.prototype.parse = function(argv) {
// trigger autocomplete first
this.autocomplete(argv);
Copy link
Collaborator

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?

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@roman-vanesyan
Copy link
Collaborator

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.

@bencao
Copy link
Author

bencao commented Feb 13, 2019

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?

@roman-vanesyan
Copy link
Collaborator

@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?

@shadowspawn
Copy link
Collaborator

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.

$ npm init -y
$ npm install commander
$ npm install omelette
$ du -d 1 -h node_modules
 76K	node_modules//commander
1.5M	node_modules//omelette
1.5M	node_modules/

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

@bencao
Copy link
Author

bencao commented Mar 5, 2019

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.

@shadowspawn shadowspawn self-requested a review May 13, 2019 10:34
@shadowspawn
Copy link
Collaborator

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

@shadowspawn
Copy link
Collaborator

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.

@shadowspawn shadowspawn mentioned this pull request Jun 23, 2019
@bencao
Copy link
Author

bencao commented Jun 23, 2019

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.

@lkloon123
Copy link

i would sugguest commander to allow plugin extensions and this would surely be great to be a plugin 👍

@jchook
Copy link

jchook commented Jan 6, 2024

@bencao Did you have any luck extracting this PR into an extension / module?

@gutenye
Copy link

gutenye commented Dec 26, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tab completion?
7 participants