cli: implement autocompletion for cli commands #132

lundibundi · 2017-05-05T18:33:38Z

Implements 'Autocompletion for CLI commands' and allows to implement 'Autocompletion for method names' (#125)

The behavior is as follows:

on empty string (or trailing space) just show what commands are available for current level
on partially entered commands show appropriate completions
on fully entered command with no trailing space or when no completions available show help
allow to use partially entered command without completing them it there are no collisions

Now it splits 'str' till limit is bound or no more separators left in 'str' if leaveEmpty is true then multiple separators in sequence are written in the resulting array as one empty string (''), else they are skipped and doesn't get counted to limit

aqrln · 2017-05-05T18:36:07Z

@lundibundi can you please add the Refs: https://github.com/metarhia/JSTP/issues/125 metadata field to the description of your second commit? Thanks!

belochub · 2017-05-05T18:37:32Z

tools/cli.js


  const result = [];
  let start = 0;

  // eslint-disable-next-line no-unmodified-loop-condition
-  for (let i = 0; !limit || i < limit; i++) {
+  while (!limit || limit - result.length > 0) {


In my opinion, rewriting second condition as

result.length < limit

will make it more readable.

Refs: #125

lundibundi · 2017-05-05T18:39:09Z

@aqrln done

belochub · 2017-05-05T18:42:43Z

tools/cli.js

    const split = str.indexOf(separator, start);
    if (split === -1) break;
-    if (!shouldTrim(start, split)) {
+    if (start !== split || leaveEmpty && !isLastEmpty(result)) {


This complex condition really needs parentheses.

aqrln · 2017-05-05T18:44:11Z

tools/cli.js

@@ -24,14 +24,64 @@ const log = (msg) => {
  if (userInput) rl.write(userInput);
 };

+function completeByFields(input, object) {
+  return Object.keys(object)
+    .filter(c => !c.startsWith('_') && (!input || c.startsWith(input)));


What type can input have? If it's always a string, then the !input check is superfluous, c.startsWith('') will always return true.

@aqrln the problem is it can be undefined when you press tab with no input, not sure how to handle this better

Ah, well then. If it can be undefined, the check is completely valid.

@aqrln, but isn't it redundant? As far as I know, String.prototype.startsWith will return false when first argument is undefined.

@aqrln, oh, I am sorry, I see why this check is OK here.

aqrln · 2017-05-05T18:46:16Z

tools/cli.js

+//  splits 'str' till limit is bound or no more separators left in 'str'
+//  if leaveEmpty is true then multiple separators in sequence are written in
+//  resulting array as one empty string (''), else they are skipped
+//  and doesn't get counted to limit
 function _split(str, separator, limit, leaveEmpty) {


Not related to this PR, but why does this function start with an underscore? It isn't exported so it's private anyway.

@aqrln we originally wrote it like this, as it really is private, i guess i will fix this during decomposition of cli.js in #127

belochub · 2017-05-05T18:46:30Z

tools/cli.js

    }
    start = split + separator.length;
  }
-  if (!shouldTrim(start, str.length)) {
+  if (start !== str.length || leaveEmpty && !isLastEmpty(result)) {


It is almost the same condition as on line 192, you should probably replace it with function, like it was before with shouldTrim.

@belochub yeah, but i'm not sure if it's okay to use shouldTrim (name) as it will also add '' when needed.

@lundibundi, I didn't say that you should leave shouldTrim the way it was before. I meant that you you should replace repetitive conditions with function, like it was done before with shouldTrim function.

@belochub I know, I meant that I couldn't come up with a good name for it.

aqrln · 2017-05-05T18:57:05Z

tools/cli.js

+}
+
+function tryComplete(input, completer) {
+  const completions = completer._complete([input], 0)[0];


As far as I understand the completer interface, _complete and _help are its public methods. Why do they start with underscores?

Ah, I see. It's a hack to make objects which serve as sources for completions completers themselves.

@aqrln hm, at first I thought to always use fields for completion, but now I see that it'd to be better to state the completions manually, then i can get rid of the underscore.

aqrln · 2017-05-05T18:58:54Z

tools/cli.js

+    }
+    return [completions, help];
+  }
+  const [newCompletions, newDepth] = completer._complete(inputs, depth);


The doc comment above this function states:

// completer - object that has '_complete(inputs, depth)' function or ._help()
// or neither (no completions or help available)

But if the completer object has no _complete method, this line will fail with a TypeError.

oh, forgot about first entrance, thanks for catching that.

aqrln · 2017-05-05T19:04:13Z

tools/cli.js

+      return helper(newDepth, depth, nextCompleter, newCompletions);
+    }
+    if (inputs[oldDepth] === completions[0]) {
+      completions.shift();


This statement modifies the completions array. Won't it have any unwanted side-effects?

it's needed to remove completion if user has already entered full command, what sort of side-effects? Actually it is possible to use it's own return statement there as completions will always by empty at that time.

Never mind, I was thinking of some global array of completions that unexpectedly gets deteriorated as user keeps hitting Tab. That's not the case here.

~~Anyway, it's not related to the current PR if that's how it is actually implemented in the CLI. It's a matter of a separate issue and PR.~~

Sorry, wrong thread.

aqrln · 2017-05-05T19:16:37Z

tools/cli.js

 commandProcessor.event = (interfaceName, eventName, args, callback) => {
  if (!state.client) return callback(new Error('Not connected'));
  state.connection.emitRemoteEvent(interfaceName, eventName, args);
  callback();
 };

+commandProcessor.event._help = () => (
+  'event <interfaceName> <eventName> [ <arg> [ , ... ] ]'


Events in JSTP don't have a concept of arguments, like methods do. They only have a single argument that can be of any type. Is it just the help that is incorrect here or that's how it is actually implemented so that it's a bug in CLI?

@aqrln not sure what you meant, the '[ <arg> [ , ... ] ]' will be passed as an array as a third argument to Connection.emitRemoteEvent

Yeah, but it's not supposed to be an array. It can be an arbitrary JavaScript value. At least, in the current version of the protocol.

Anyway, it's not related to the current PR if that's how it is actually implemented in the CLI. It's a matter of a separate issue and PR.

yeah, it is implemented like that, so we should open an issue for that.

If anyone passed 'completer' that has no _complete method it'd have failed with TypeError

aqrln · 2017-05-05T19:18:05Z

tools/cli.js

 function _split(str, separator, limit, leaveEmpty) {
-  const shouldTrim = (start, split) => !leaveEmpty && start === split;
+  const isLastEmpty = arr => !arr[arr.length - 1];

  const result = [];
  let start = 0;

  // eslint-disable-next-line no-unmodified-loop-condition


Does the linter still fail without this directive with the new loop condition?

@aqrln yeah(

Get rid of underscore methods and state available completions manually in complete method

* don't use shift, just return empty array * extract complex condition to separate arrow function

belochub · 2017-05-05T20:46:32Z

tools/cli.js

+// limit - resulting length of output array - 1 (last one is what's left),
+//         if !limit === true => means no limit and split till no more
+//         separators found
+// leaveEmpty - if true multiple separators in sequence will be added as empty


Word 'empty' is repeated twice here, probably a typo.

aqrln · 2017-05-05T20:54:22Z

tools/cli.js

@@ -109,17 +109,18 @@ const state = {
  connection: null
 };

-commandProcessor._complete = (inputs, depth) => {
+commandProcessor.complete = (inputs, depth) => {
+  const completions = ['call', 'event', 'connect', 'disconnect', 'exit'];


~~I'd prefer these to be sorted alphabetically, though I don't feel strongly for it.~~

aqrln · 2017-05-05T20:54:55Z

tools/cli.js

 const state = {
  client: null,
  connection: null
 };

+commandProcessor.complete = (inputs, depth) => {
+  const completions = ['call', 'event', 'connect', 'disconnect', 'exit'];


I'd prefer these to be sorted alphabetically, though I don't feel strongly for it.

* sort available completions in commandProcessor * fix a typo in iterativeCompletion doc

It was working incorrectly with multiple leading separators

belochub

LGTM

aqrln

LGTM

Now it splits 'str' till limit is bound or no more separators left in 'str' if leaveEmpty is true then multiple separators in sequence are written in the resulting array as one empty string (''), else they are skipped and doesn't get counted to limit PR-URL: #132 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

PR-URL: #132 Refs: #125 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

aqrln · 2017-05-05T22:35:53Z

Landed in f11130f and 8a6bc52 🎉

Now it splits 'str' till limit is bound or no more separators left in 'str' if leaveEmpty is true then multiple separators in sequence are written in the resulting array as one empty string (''), else they are skipped and doesn't get counted to limit PR-URL: #132 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

PR-URL: #132 Refs: #125 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Now it splits 'str' till limit is bound or no more separators left in 'str' if leaveEmpty is true then multiple separators in sequence are written in the resulting array as one empty string (''), else they are skipped and doesn't get counted to limit PR-URL: #132 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

PR-URL: #132 Refs: #125 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Now it splits 'str' till limit is bound or no more separators left in 'str' if leaveEmpty is true then multiple separators in sequence are written in the resulting array as one empty string (''), else they are skipped and doesn't get counted to limit PR-URL: metarhia/jstp#132 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

PR-URL: metarhia/jstp#132 Refs: metarhia/jstp#125 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

Now it splits 'str' till limit is bound or no more separators left in 'str' if leaveEmpty is true then multiple separators in sequence are written in the resulting array as one empty string (''), else they are skipped and doesn't get counted to limit PR-URL: metarhia/jstp#132 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

PR-URL: metarhia/jstp#132 Refs: metarhia/jstp#125 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

cli: modify split behaviour

c917944

Now it splits 'str' till limit is bound or no more separators left in 'str' if leaveEmpty is true then multiple separators in sequence are written in the resulting array as one empty string (''), else they are skipped and doesn't get counted to limit

lundibundi added the cli label May 5, 2017

lundibundi self-assigned this May 5, 2017

lundibundi requested review from belochub and aqrln May 5, 2017 18:33

aqrln mentioned this pull request May 5, 2017

Autocompletion in CLI #125

Closed

2 tasks

belochub reviewed May 5, 2017

View reviewed changes

cli: implement autocompletion for cli commands

f5b461f

Refs: #125

lundibundi force-pushed the autocompletion-cli branch from e2d1b75 to f5b461f Compare May 5, 2017 18:38

belochub reviewed May 5, 2017

View reviewed changes

aqrln reviewed May 5, 2017

View reviewed changes

belochub reviewed May 5, 2017

View reviewed changes

aqrln reviewed May 5, 2017

View reviewed changes

cli: fix TypeError in iterativeCompletion

65e81ec

If anyone passed 'completer' that has no _complete method it'd have failed with TypeError

aqrln reviewed May 5, 2017

View reviewed changes

aqrln changed the title ~~Autocompletion cli~~ cli: implement autocompletion for cli commands May 5, 2017

aqrln added this to the 1.0.0 milestone May 5, 2017

lundibundi added 2 commits May 5, 2017 22:35

cli: refactor completion interface

853d867

Get rid of underscore methods and state available completions manually in complete method

cli: refactor split and iterativeCompletion

58ffd78

* don't use shift, just return empty array * extract complex condition to separate arrow function

belochub reviewed May 5, 2017

View reviewed changes

aqrln reviewed May 5, 2017

View reviewed changes

lundibundi added 2 commits May 6, 2017 00:27

cli: fix stylistic mistakes

de61b6e

* sort available completions in commandProcessor * fix a typo in iterativeCompletion doc

cli: fix _split behavior

ad1e61a

It was working incorrectly with multiple leading separators

lundibundi force-pushed the autocompletion-cli branch from 20946fe to ad1e61a Compare May 5, 2017 21:54

belochub approved these changes May 5, 2017

View reviewed changes

aqrln approved these changes May 5, 2017

View reviewed changes

aqrln pushed a commit that referenced this pull request May 5, 2017

cli: implement autocompletion for cli commands

8a6bc52

PR-URL: #132 Refs: #125 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

aqrln closed this May 5, 2017

aqrln deleted the autocompletion-cli branch May 5, 2017 22:36

aqrln mentioned this pull request May 5, 2017

Arbitrary event argument type in CLI #134

Closed

belochub pushed a commit that referenced this pull request Jan 22, 2018

cli: implement autocompletion for cli commands

0847bfc

PR-URL: #132 Refs: #125 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

belochub pushed a commit that referenced this pull request Jan 22, 2018

cli: implement autocompletion for cli commands

2893994

PR-URL: #132 Refs: #125 Reviewed-By: Mykola Bilochub <nbelochub@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>

belochub mentioned this pull request Jan 22, 2018

v1.0.0 proposal #311

Merged

cli: implement autocompletion for cli commands #132

cli: implement autocompletion for cli commands #132

Conversation

lundibundi commented May 5, 2017 • edited Loading

aqrln commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belochub left a comment

Choose a reason for hiding this comment

aqrln left a comment

Choose a reason for hiding this comment

aqrln commented May 5, 2017

lundibundi commented May 5, 2017 •

edited

Loading

lundibundi May 5, 2017 •

edited

Loading

aqrln May 5, 2017 •

edited

Loading

lundibundi May 5, 2017 •

edited

Loading

aqrln May 5, 2017 •

edited

Loading