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

repl: add autocomplete for filesystem modules #26648

Closed
wants to merge 1 commit into from

Conversation

antsmartian
Copy link
Contributor

@antsmartian antsmartian commented Mar 14, 2019

This is a implementation of: #17764

There are other few things to be done like handling autocompletion for filenames etc.
Marking it as WIP. But would love to get feedback from @nodejs/repl

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Mar 14, 2019
lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
@antsmartian antsmartian added the wip Issues and PRs that are still a work in progress. label Mar 14, 2019
@antsmartian antsmartian removed the wip Issues and PRs that are still a work in progress. label Mar 20, 2019
@antsmartian
Copy link
Contributor Author

@devsnek @richardlau @nodejs/repl PTAL..

@antsmartian antsmartian changed the title (WIP) repl: add autocomplete for filesystem modules repl: add autocomplete for filesystem modules Mar 20, 2019
@antsmartian
Copy link
Contributor Author

lib/repl.js Outdated Show resolved Hide resolved
test/parallel/test-repl-tab-complete.js Outdated Show resolved Hide resolved
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is overall looking very promising. I believe this should be good to go after addressing my comments.

lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
test/parallel/test-repl-tab-complete.js Outdated Show resolved Hide resolved
@antsmartian
Copy link
Contributor Author

@BridgeAR Thanks much for the follow up and I'm sorry for being late here. I have fixed up the comments, could you please have a look at them?

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with my comment addressed.

lib/repl.js Outdated Show resolved Hide resolved
@antsmartian
Copy link
Contributor Author

@BridgeAR Have fixed your comments, PTAL.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I found one more small thing that won't change the behavior but filter should always be a string or undefined. Using false adds another map (internal V8 type, also called hidden class) to it.

Thanks for following up on the comments!

lib/repl.js Outdated

let filePath = match[1];
let fileList;
filter = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
filter = false;
filter = '';

@nodejs-github-bot
Copy link
Collaborator

@antsmartian
Copy link
Contributor Author

@BridgeAR @nodejs/repl PTAL.. Thanks!

@antsmartian antsmartian added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2019
@nodejs-github-bot
Copy link
Collaborator

@antsmartian
Copy link
Contributor Author

antsmartian commented Apr 27, 2019

@nodejs/repl Can I get one more review, please. Planning to land by April 29th if there are no objections.

@nodejs-github-bot
Copy link
Collaborator

@antsmartian
Copy link
Contributor Author

@BridgeAR I have addressed your comments regarding filter, PTAL and see if its looks good.. Thanks!

@BridgeAR
Copy link
Member

Still LGTM

@antsmartian
Copy link
Contributor Author

Landed in d69f004 🎉

@antsmartian antsmartian deleted the add_filelist_tab branch April 29, 2019 02:00
antsmartian added a commit that referenced this pull request Apr 29, 2019
PR-URL: #26648
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Apr 29, 2019
PR-URL: #26648
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos targos mentioned this pull request May 6, 2019
@addaleax addaleax added backport-blocked-v12.x notable-change PRs with changes that should be highlighted in changelogs. and removed backport-blocked-v12.x labels May 6, 2019
targos added a commit that referenced this pull request May 7, 2019
Notable changes:

* deps:
  * Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
    parser refuse any request URL that contained the "|" (vertical bar)
    character. #27595
* tls:
  * Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
    option to `tls.createServer()`. When enabled, TSL packet trace
    information is written to `stderr`. This can be used to debug TLS
    connection problems. #27497
    #27376
* cli:
  * Added a `--trace-tls` command-line flag that enables tracing of TLS
    connections without the need to modify existing application code.
    #27497
  * Added a `--cpu-prof-interval` command-line flag. It can be used to
    specify the sampling interval for the CPU profiles generated by
    `--cpu-prof`. #27535
* module:
  * Added the `createRequire()` method. It allows to create a require
    function from a file URL object, a file URL string or an absolute
    path string. The existing `createRequireFromPath()` method is now
    deprecated #27405.
  * Throw on `require('./path.mjs')`. This is technically a breaking
    change that should have landed with Node.js 12.0.0. It is necessary
    to have this to keep the possibility for a future minor version to
    load ES Modules with the require function.
    #27417
* repl:
  * The REPL now supports multi-line statements using `BigInt` literals
    as well as public and private class fields and methods.
    #27400
  * The REPL now supports tab autocompletion of file paths with `fs`
    methods. #26648
* meta:
  * Added Christian Clauss (https://github.com/cclauss) to
    collaborators. #27554

PR-URL: #27578
targos added a commit that referenced this pull request May 7, 2019
Notable changes:

* deps:
  * Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
    parser refuse any request URL that contained the "|" (vertical bar)
    character. #27595
* tls:
  * Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
    option to `tls.createServer()`. When enabled, TSL packet trace
    information is written to `stderr`. This can be used to debug TLS
    connection problems. #27497
    #27376
* cli:
  * Added a `--trace-tls` command-line flag that enables tracing of TLS
    connections without the need to modify existing application code.
    #27497
  * Added a `--cpu-prof-interval` command-line flag. It can be used to
    specify the sampling interval for the CPU profiles generated by
    `--cpu-prof`. #27535
* module:
  * Added the `createRequire()` method. It allows to create a require
    function from a file URL object, a file URL string or an absolute
    path string. The existing `createRequireFromPath()` method is now
    deprecated #27405.
  * Throw on `require('./path.mjs')`. This is technically a breaking
    change that should have landed with Node.js 12.0.0. It is necessary
    to have this to keep the possibility for a future minor version to
    load ES Modules with the require function.
    #27417
* repl:
  * The REPL now supports multi-line statements using `BigInt` literals
    as well as public and private class fields and methods.
    #27400
  * The REPL now supports tab autocompletion of file paths with `fs`
    methods. #26648
* meta:
  * Added Christian Clauss (https://github.com/cclauss) to
    collaborators. #27554

PR-URL: #27578
@rauschma
Copy link

rauschma commented May 18, 2019

Small bug on macOS (when, e.g., in /Users/rauschma):

fs.readFileSync('Pub<tab>

EXPECTED: fs.readFileSync('Public
ACTUAL:   fs.readFileSync('Pubublic

@jabr
Copy link

jabr commented Jul 19, 2019

Not sure if it counts as a bug, but it doesn't work with fs.promises. functions.

@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. notable-change PRs with changes that should be highlighted in changelogs. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants