RFC: Revamp the REPL Tab completion feature #17826

Bamieh · 2017-12-22T18:12:41Z

This feature reworks the tab complete feature in the repl module.

Rather than using regular expressions to guess the completion options, the tab completion will call a method on the function through a Symbol attached on these methods, which returns an array of possible completion options.

const repl = require('repl');
var fnWithTabComplete = function(someParam) {
   //fn body
}

fnWithTabComplete[repl.replComplete] = function(inputParams, paramsText) {
  // inputParams => [param1, param2, ...]
  // paramsText => "param1, param2 ..."

  return ['tabComplete1', 'tabComplete2'];
}


fnWithTabComplete('...<Tab>'
/*
  tabComplete1    tabComplete2
*/

Notes

This tab completion feature works on all methods and functions with the repl.replComplete Symbol attached on them.
The completer function gets an array of the parameters the user added. This way the implementers can specify different tab completion based on the parameter the user is trying auto complete.

Benefits

more scalable than using regex
exposes the tab completion for both external and internal modules
move tab completion implementations into the concerned modules rather than bloating the repl module.
Implementing tab completion in other modules becomes a breeze rather than trying to craft a usually-morbid regular expression.
Additionally, using the current approach for tab completion renders the feature rigid:
- When I added the tab completion feature for the fs module methods, a concern about inconsistency of having the tab completion work in some places while not in others. This approach makes it the concern of the consuming modules to implement the tab completion as needed rather than having it
- All the methods supported by tab completion will have to be maintained by the repl module, its test cases, and any updated on the main module will have to be reflected. Basically the repl will have a large dictionary ot method names in other modules.

Context

Recently i submitted a PR to implement tab completion for the fs module, and i was faced with many design issues mainly because of how impractical the regexp approach is for expanding the tab completion feature.

@starkwang @apapirovski @benjamingr can you provide feedback on this please, since you are in context with this issue already. Thanks!

Semver: Major or Minor

I believe this does not introduce any breaking changes so it could be introduced as a minor semver.

RFC

I am requesting comments and feedback on the approach and some general guidance. This PR is far from complete but it contains the initial direction of implementation. Also I have an already opened PR for the fs path autocomplete, if this PR receives positive feedback i will refactor the other one to use this feature.

Final notes

Finally, I will leave you with this piece of wisdom my brain crafted while implementing the feature 😄

I feel that the regular expression approach is similar to the foreign key constrain in SQL, using constrains usually introduces rigidity and impracticality.

Cheers!

addaleax

Nice work so far, I like it! 👍

Do you have a specific list of functions inside Node core where this could be added?

Also, can you add documentation for repl.replComplete?

addaleax · 2017-12-22T18:43:34Z

lib/repl.js

@@ -964,6 +964,9 @@ function complete(line, callback) {
  // REPL commands (e.g. ".break").
  var filter;
  var match = null;
+
+  var functionCallRE = /\b(\w+)\([^)]*$/;


I think there can be arbitrary whitespace before the opening (?

Yes. I will add test cases and go through the code again to make sure its production ready. meanwhile i just wanted to submit the PR to get feedback for the general direction of the feature rather than the code details.

Does (\w+) catch all valid function names?

Misses $, but otherwise seems OK. % may also be needed for natives support.

nit: this can be const

devsnek · 2017-12-22T18:50:16Z

lib/repl.js

+// console.log("::a, b = 0, /*c,*/ d::", annotate("a, b = 0, /*c,*/ d"))
+// console.log("::a, \"bamieh, ahm\", \"incom::", annotate("a, \"bamieh, ahm\", \"incom"))
+
+exports.replComplete = Symbol('REPL Complete');


i feel like this name is a bit unclear, maybe tabComplete and REPL Tab Complete?

thanks for the review! As i said the code still needs some work, but i wanted to put it out ASAP to get feedback in order to know if i should put in more time into this feature

Bamieh · 2017-12-22T19:36:22Z

@addaleax Awesome, I wanted to make sure that the feature is aligned with node. I will add test cases, update the docs, rename things and rewrite some parts to make sure its production ready

starkwang · 2017-12-23T14:58:13Z

lib/repl.js

+  let started = false;
+  return params.split(',').reduce((acc, cur, i, array) => {
+     if(/'|"|`|\/\*|\*\//.test(cur)) {
+        if(started === true) {


Here can be simplified to if(started) { ... }

starkwang · 2017-12-23T15:06:54Z

lib/repl.js

+        try {
+          var args = parseInputParameteres(paramsPassed);
+          console.log('paramsPassed::', paramsPassed)
+          console.log('args::', args)


Can we remove these two console.log ? 😉

starkwang · 2017-12-23T15:14:51Z

lib/repl.js

+  } else if (line.match(functionCallRE)) {
+    const fnParenthesesIndex = line.lastIndexOf('(');
+    const fullMethod = line.slice(0, fnParenthesesIndex);
+    const paramsPassed = line.slice(fnParenthesesIndex+1) || '';


eslint:
line.slice(fnParenthesesIndex+1) -> line.slice(fnParenthesesIndex + 1)

jasnell · 2017-12-28T17:21:40Z

lib/repl.js

+
+    this.eval(evalExpr, this.context, 'repl', (e, completer) => {
+      var completionGroup = [];
+      if(typeof completer === 'function') {


nit: be sure to run make lint ... there needs to be a space between the if and the ( here :-)

Bamieh · 2017-12-28T18:03:25Z

@Jansell sorry i have been a little busy, but this PR is undergoing major rework. again i submitted the PR to request comments on the concept not the actual code. Hopefully I will submit my final iteration later on tomorrow.

BridgeAR · 2018-02-07T03:51:02Z

CI https://ci.nodejs.org/job/node-test-pull-request/12991/

BridgeAR · 2018-02-09T02:57:43Z

@Bamieh it seems like there are some linting issues and a test it constantly failing:

not ok 1364 parallel/test-repl-tab-complete
  ---
  duration_ms: 0.313
  severity: fail
  stack: |-
    [ [ 13, 7 ], undefined ]
    [ [ 13, 7 ], undefined ]
    assert.js:527
        throw newErr;
        ^
    
    AssertionError [ERR_ASSERTION]: ifError got unwanted exception: 0 strictEqual 1
        at Domain.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-repl-tab-complete.js:49:10)
        at Domain.emit (events.js:136:15)
        at Domain.emit (domain.js:421:20)
        at Domain._errorHandler (domain.js:203:23)
        at Object.setUncaughtExceptionCaptureCallback (domain.js:119:29)
        at process._fatalException (bootstrap_node.js:441:31)
        at testMe.complete.common.mustCall (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-repl-tab-complete.js:276:16)
  ...

Would you be so kind and have a look? :-)

BridgeAR · 2018-02-16T23:35:54Z

lib/internal/repl/parser.js

+const walk = require('internal/deps/acorn/dist/walk');
+
+function parseFunctionArgs(input, r) {
+  if(input === '') return [''];


Nit: Please add a empty space after the if in all cases (so also below).

BridgeAR · 2018-02-16T23:36:34Z

lib/internal/repl/parser.js

+      const trimmedStr = input.trim();
+      const lastArgIndex = input.lastIndexOf(',');
+      const lastArg = input.substr(lastArgIndex + 1).trim();
+      return parseFunctionArgs(input.substring(0, lastArgIndex), lastArg);


The indentation looks wrong here. Does the linter pass here?

BridgeAR · 2018-02-16T23:36:57Z

lib/internal/repl/parser.js

+  }
+}
+
+


Non blocking nit: please only use a single new line.

BridgeAR · 2018-02-16T23:38:07Z

test/parallel/test-repl-parser.js

+
+  inputs.forEach(input => {
+    const results = parseFunctionArgs(input.args);
+    assert.strictEqual(results.length, input.expectedResult.length);


This is a obsolete check because the next line will verify that the input and result is identical.

BridgeAR · 2018-02-16T23:39:04Z

test/parallel/test-repl-tab-complete.js

+// tab completion Symbol
+{
+  assert.strictEqual(typeof repl.tabComplete, 'symbol');
+}


This test does not need a own scope as it does not interfere with anything else.

BridgeAR · 2018-02-16T23:43:30Z

test/parallel/test-repl-tab-complete.js

@@ -203,6 +203,86 @@ testMe.complete('toSt', common.mustCall(function(error, data) {
  assert.deepStrictEqual(data, [['toString'], 'toSt']);
 }));

+// tab completion Symbol


Nit: Please always use a capital letter as first character of a comment and punctuate them.

BridgeAR · 2018-03-02T04:13:00Z

Ping @Bamieh

Bamieh · 2018-03-05T12:38:26Z

@BridgeAR please keep the PR opened, I have it scheduled on friday to finish it.

BridgeAR · 2018-04-16T03:11:35Z

Closing due to long inactivity. @Bamieh please feel free to open a new PR or to leave a comment so this PR can be reopened in case you would like to continue working on this.

initial feature path

e15b3d1

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Dec 22, 2017

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 22, 2017

addaleax reviewed Dec 22, 2017

View reviewed changes

devsnek reviewed Dec 22, 2017

View reviewed changes

starkwang reviewed Dec 23, 2017

View reviewed changes

jasnell reviewed Dec 28, 2017

View reviewed changes

Bamieh added 4 commits January 1, 2018 23:20

refactors

8b92e20

split branches

d1e86cb

revert acron_loose changes

1b6e0fe

tidy up

2b46a73

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR requested review from lance, jasnell and TimothyGu February 7, 2018 03:49

BridgeAR reviewed Feb 16, 2018

View reviewed changes

Bamieh mentioned this pull request Feb 18, 2018

Tab complete paths for fs functions in repl #17790

Closed

3 tasks

BridgeAR closed this Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Revamp the REPL Tab completion feature #17826

RFC: Revamp the REPL Tab completion feature #17826

Bamieh commented Dec 22, 2017 •

edited

Loading

addaleax left a comment

addaleax Dec 22, 2017

Bamieh Dec 22, 2017

benjamingr Dec 24, 2017

Jessidhia Dec 25, 2017

jasnell Dec 28, 2017

devsnek Dec 22, 2017

Bamieh Dec 22, 2017

Bamieh commented Dec 22, 2017

starkwang Dec 23, 2017

starkwang Dec 23, 2017

starkwang Dec 23, 2017

jasnell Dec 28, 2017

Bamieh commented Dec 28, 2017

BridgeAR commented Feb 7, 2018

BridgeAR commented Feb 9, 2018

BridgeAR Feb 16, 2018

BridgeAR Feb 16, 2018

BridgeAR Feb 16, 2018

BridgeAR Feb 16, 2018

BridgeAR Feb 16, 2018

BridgeAR Feb 16, 2018

BridgeAR commented Mar 2, 2018

Bamieh commented Mar 5, 2018

BridgeAR commented Apr 16, 2018

RFC: Revamp the REPL Tab completion feature #17826

RFC: Revamp the REPL Tab completion feature #17826

Conversation

Bamieh commented Dec 22, 2017 • edited Loading

Notes

Benefits

Context

Semver: Major or Minor

RFC

Final notes

addaleax left a 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

Bamieh commented Dec 22, 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

Bamieh commented Dec 28, 2017

BridgeAR commented Feb 7, 2018

BridgeAR commented Feb 9, 2018

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

BridgeAR commented Mar 2, 2018

Bamieh commented Mar 5, 2018

BridgeAR commented Apr 16, 2018

Bamieh commented Dec 22, 2017 •

edited

Loading