Tab complete paths for `fs` functions in repl #17790

Bamieh · 2017-12-20T15:15:00Z

Adds autocomplete in repl for fs methods that lookup file paths. Closes #17764

Tab complete only works for relative paths (this approach made most sense for me)
The fs module must be explicitly accessed by fs.

var myFS = require('fs');
myFS.readFile('...<Tab>' // will not work

I was afraid to refactor the require('...<Tab>') completion into a separate function and call it in both the require and fs.<method>, this would tidy up the code a little. I am willing to refactor it if I get a green light.
I have the methods readlink and readlinkSync in the supported tab complete methods. I am not sure if these make sense to be added, or need a special lookup for only symlinks.

P.S.
Implementing a feature to track all the variables defined for tab completion would benefit the repl tab completion in two folds:

the tab completion will be smarter to know which variable refers to the fs module.
all definitions (const and let) will be supported in the tab completion.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

repl

Fixes nodejs#17764

Fishrock123 · 2017-12-20T16:05:19Z

lib/repl.js

+].join('|');
+
+const fsRE = new RegExp(
+  `fs\\.(?:${fsTabbableMethods})\\(["'](([\\w@./-]+\\/)?(?:[\\w@./-]*))`


Does it need the fs. restriction?

@Fishrock123 the restriction is not needed, but its there to avoid unexpected collisions for functions with similar names.

function readFile(fileName) { } readFile('...<Tab>' // this will work and might be unexpected for users

this is my opinion, what do you think?

Do you think people are really likely to in the repl during an existing line-statement when they don't want autocomplete?

I personally agree with @Fishrock123 and think it is better to keep this open. A false positive does not hurt here out of my perspective.

Fishrock123 · 2017-12-20T16:06:31Z

Also, going to echo ben's comment from the issue:

Just noting that auto-completion of file names in large directories will freeze the REPL until #15699 is resolved.

The fact that it is on tab should avoid any unexpected behavior though, I think? How does this perform on, say, the test/parallel/ directory?

apapirovski · 2017-12-20T16:15:55Z

lib/repl.js

+  } else if (match = (line.match(requireRE) || line.match(fsRE))) {
+    // require('...<Tab>') and fs.<method>('...<Tab>')
+
+    const isFS = line.match(fsRE);


This RegExp looks pretty intensive to me, we should not be running it twice. The if statement could be refactored to be (matchRequire = line.match(requireRE)) || (matchFs = line.match(fsRE)). Then we can assign match within the body.

(Of course, the var declaration will be needed for both.)

definitely.

apapirovski · 2017-12-20T16:18:59Z

lib/repl.js

+  'unlink',
+  'unlinkSync',
+  'utimes',
+  'utimesSync',


This will become a huge pain to maintain over time, especially since the test also duplicates it.

@apapirovski thanks for the review! i thought of exporting it in the repl to reduce this maintenance cost, but i was unsure about it. Do you have any other suggestions?

@apapirovski @benjamingr An alternative implementation of regex matching is to have a Symbol for tabCompletion in node. Then each module would add their own implementation of the tab completion on the method level (also allowing non-core modules to enjoy this feature as well).

That would actually be awesome and a nice idea.

Would checking the exports for functions be enough? The regex then already specifies it must be within a string...

I mean I sort of like the idea of a custom tabCompletion symbol for repl but I don't like the idea of shipping that code to everyone that doesn't even use the repl (it's always there on the prototype, for no reason).

Maybe the answer would be to have some sort of a definitions file that repl includes that enhances any supported modules. Like internals/repl/definitions/*.js (stream, fs, etc.) or something. Just musing out loud...

Although I guess it depends on how the tab completion works, per method, or per module... Anyway, move along, nothing to see here... just rambling. 😆

Would it be possible to create the list dynamically? e.g.

Object.keys(fs).filter(name => typeof fs[name] === 'function' && !name.startsWith('_'))

That would enable a few methods that aren't currently in this PR (e.g. writeFileSync), although it might cause a false positive if there are any methods on fs that don't accept a filepath as their first argument.

Personally, I think we could be liberal about enabling tab-completion, and just apply it to all fs methods. (By comparison, Bash always tab-completes arguments that look like filepaths, even though it doesn't know the semantics of the command being used.)

devsnek · 2017-12-20T16:26:41Z

looking at this behavior in a more general context, if i see fs.unlink work, but then perhaps i want to use asar.unpack or const unlink = util.promisify(fs.unlink) or anything else that takes a path, all of a sudden this behavior is gone. That seems like it will be more annoying than anything else.

devsnek · 2017-12-20T16:32:30Z

if/once scandir lands a more general "if in string and thing typed looks like a path" feature could be implemented

Bamieh · 2017-12-20T16:57:28Z

@Fishrock123 I have read that comment, but it is the same for the require tab completion as well. both require and fs are using the same code now, so implementing the fs.scandir method will fix it for both.

Bamieh · 2017-12-20T17:06:22Z

@devsnek

if/once scandir lands a more general "if in string and thing typed looks like a path" feature could be implemented

I do not see this feature possible with the current regex architecture of the repl. for example what would a tab completion for a .<Tab> be? this feature might create a lot of unexpected and untestable scenarios.

looking at this behavior in a more general context, if i see fs.unlink work, but then perhaps i want to use asar.unpack or const unlink = util.promisify(fs.unlink) or anything else that takes a path, all of a sudden this behavior is gone. That seems like it will be more annoying than anything else.

Then we should add code for the repl to support these functions as well. The code could be easily refactored to be extendable by other modules.

devsnek · 2017-12-20T17:13:31Z

Then we should add code for the repl to support these functions as well. The code could be easily refactored to be extendable by other modules.

like the author of the imgur module adds regex for their module and the author of the asar module adds regex for their module? I don't understand how any way of locking this feature to specific methods or whatever else creates a positive experience for someone using the repl. imo this is better suited to not be a feature at all if it can't be more generally applied.

tab completion for ". would list files and directories in .. is there another behavior that would be expected?

bnoordhuis · 2017-12-20T21:48:14Z

How does this perform on, say, the test/parallel/ directory?

That's the lightest of benchmarks, test/parallel has < 2,000 files in it. I regularly work inside directories that have > 200,000 files in them.

That said, it probably won't be too bad in the common case. 250k files on a SSD take about 1-3 seconds to fs.readdirSync() on my system, depending on whether the cache is warmed up or not.

(The cold case on spinning platters probably won't do so well, though.)

starkwang · 2017-12-21T01:40:10Z

lib/repl.js

+].join('|');
+
+const fsRE = new RegExp(
+  `fs\\.(?:${fsTabbableMethods})\\(["'](([\\w@./-]+\\/)?(?:[\\w@./-]*))`


It's better to add a \b at the beginning of the RegExp, otherwise it will match something like xxxxfs.readFile("

BridgeAR · 2018-02-16T23:48:25Z

lib/repl.js

+].join('|');
+
+const fsRE = new RegExp(
+  `fs\\.(?:${fsTabbableMethods})\\(["'](([\\w@./-]+\\/)?(?:[\\w@./-]*))`


I personally agree with @Fishrock123 and think it is better to keep this open. A false positive does not hurt here out of my perspective.

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

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

+// Test tab complete for fs module
+putIn.run(['.clear']);
+
+// does not auto complete empty paths


Please always use a capital letter as first character in a comment and punctuate them.

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

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

+    'unlinkSync',
+    'utimes',
+    'utimesSync',
+  ];


I think the same applies here as above: using a dynamical filtered list is probably better than a static one.

BridgeAR · 2018-02-16T23:50:56Z

@Bamieh do you still work on this?

Bamieh · 2018-02-18T08:57:01Z

@BridgeAR I will rework it when #17826 is done using the introduced tab completion symbol. I left this open cuz that PR is requesting comments still. I'll close and re-open when ready

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

Fixes nodejs#17764

dc1c091

Fixes nodejs#17764

Bamieh force-pushed the repl-fs-autocomplete branch from 65f3d29 to dc1c091 Compare December 20, 2017 15:15

Fishrock123 reviewed Dec 20, 2017

View reviewed changes

apapirovski reviewed Dec 20, 2017

View reviewed changes

starkwang reviewed Dec 21, 2017

View reviewed changes

Bamieh mentioned this pull request Dec 22, 2017

RFC: Revamp the REPL Tab completion feature #17826

Closed

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 reviewed Feb 16, 2018

View reviewed changes

Bamieh closed this Feb 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tab complete paths for `fs` functions in repl #17790

Tab complete paths for `fs` functions in repl #17790

Bamieh commented Dec 20, 2017

Fishrock123 Dec 20, 2017

Bamieh Dec 20, 2017 •

edited

Loading

Fishrock123 Dec 20, 2017

BridgeAR Feb 16, 2018

Fishrock123 commented Dec 20, 2017

apapirovski Dec 20, 2017

Bamieh Dec 20, 2017

apapirovski Dec 20, 2017

Bamieh Dec 20, 2017

Bamieh Dec 20, 2017

benjamingr Dec 20, 2017

Fishrock123 Dec 20, 2017

apapirovski Dec 20, 2017 •

edited

Loading

not-an-aardvark Dec 21, 2017

devsnek commented Dec 20, 2017 •

edited

Loading

devsnek commented Dec 20, 2017

Bamieh commented Dec 20, 2017

Bamieh commented Dec 20, 2017

devsnek commented Dec 20, 2017

bnoordhuis commented Dec 20, 2017

starkwang Dec 21, 2017 •

edited

Loading

BridgeAR Feb 16, 2018

BridgeAR Feb 16, 2018

BridgeAR Feb 16, 2018

BridgeAR commented Feb 16, 2018

Bamieh commented Feb 18, 2018

Tab complete paths for fs functions in repl #17790

Tab complete paths for fs functions in repl #17790

Conversation

Bamieh commented Dec 20, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Bamieh Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Dec 20, 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

apapirovski Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Dec 20, 2017 • edited Loading

devsnek commented Dec 20, 2017

Bamieh commented Dec 20, 2017

Bamieh commented Dec 20, 2017

devsnek commented Dec 20, 2017

bnoordhuis commented Dec 20, 2017

starkwang Dec 21, 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

BridgeAR commented Feb 16, 2018

Bamieh commented Feb 18, 2018

Tab complete paths for `fs` functions in repl #17790

Tab complete paths for `fs` functions in repl #17790

Bamieh Dec 20, 2017 •

edited

Loading

apapirovski Dec 20, 2017 •

edited

Loading

devsnek commented Dec 20, 2017 •

edited

Loading

starkwang Dec 21, 2017 •

edited

Loading