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.

Sign up for GitHub

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

Jump to bottom

repl: include folder extensions in autocomplete #14727

Closed
wants to merge 0 commits into from
Closed

repl: include folder extensions in autocomplete #14727

wants to merge 0 commits into from

Conversation

not-an-aardvark
Copy link
Contributor

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

When autocompleting require calls, the repl strips .js file extensions from results. However, stripping an extension from a directory results in an error when require is called. Update the autocompletion logic to avoid stripping extensions from directories.

Fixes: #14726

@not-an-aardvark not-an-aardvark added the repl Issues and PRs related to the REPL subsystem. label Aug 10, 2017
Trott
Trott approved these changes Aug 10, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI passes

@not-an-aardvark
Copy link
Contributor Author

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

cjihrig
cjihrig approved these changes Aug 10, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. It might be possible to reuse existing fixtures instead of creating new ones, but not a big deal.

EDIT: Disregard my comment about reusing an existing fixture :-)

not-an-aardvark
not-an-aardvark commented Aug 10, 2017
View reviewed changes
lib/repl.js Outdated
@@ -851,14 +851,15 @@ function complete(line, callback) {
// Exclude versioned names that 'npm' installs.
continue;
}
if (exts.indexOf(ext) !== -1) {
abs = path.resolve(dir, name);
isDirectory = fs.statSync(abs).isDirectory();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this again, I'm wondering if this should be moved back into a try-catch to avoid creating an error if the path doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

It would be a race condition that is hart to trigger but as there was a guard for this before I would say it should be kept as it was.

BridgeAR
BridgeAR requested changes Aug 10, 2017
View reviewed changes
lib/repl.js Outdated
@@ -851,14 +851,15 @@ function complete(line, callback) {
// Exclude versioned names that 'npm' installs.
continue;
}
if (exts.indexOf(ext) !== -1) {
abs = path.resolve(dir, name);
isDirectory = fs.statSync(abs).isDirectory();
Copy link
Member

Choose a reason for hiding this comment

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

It would be a race condition that is hart to trigger but as there was a guard for this before I would say it should be kept as it was.

lib/repl.js Outdated
if (!subdir || base !== 'index') {
group.push(subdir + base);
}
} else {
abs = path.resolve(dir, name);
try {
Copy link
Member

Choose a reason for hiding this comment

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

The try should be moved into the if statement as it is not necessary for non directories.

@not-an-aardvark
Copy link
Contributor Author

New CI: https://ci.nodejs.org/job/node-test-pull-request/9614/console

BridgeAR
BridgeAR approved these changes Aug 11, 2017
View reviewed changes
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 a nit.

lib/repl.js Outdated
if (!subdir || base !== 'index') {
group.push(subdir + base);
}
} else {
abs = path.resolve(dir, name);
} else if (isDirectory) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you could get rid of one of the isDirectory checks (the churn is there anyway).

if (isDirectory) {
  // ...
} else if (exts.includes(ext)) {
  // ...
}

@not-an-aardvark
Copy link
Contributor Author

CI again: https://ci.nodejs.org/job/node-test-pull-request/9615/console

aqrln
aqrln approved these changes Aug 11, 2017
View reviewed changes
Copy link
Contributor

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

Still LGTM 👍

@not-an-aardvark not-an-aardvark force-pushed the repl-autocomplete-folder-extensions branch from 31e2174 to b646a3d Compare August 13, 2017 08:10
not-an-aardvark added a commit to not-an-aardvark/node that referenced this pull request Aug 13, 2017
@not-an-aardvark
repl: include folder extensions in autocomplete
b646a3d 
When autocompleting `require` calls, the repl strips .js file extensions
from results. However, stripping an extension from a directory results
in an error. Update the autocompletion logic to avoid stripping
extensions from directories.

PR-URL: nodejs#14727
Fixes: nodejs#14726
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
@not-an-aardvark
Copy link
Contributor Author

Landed in b646a3d

@not-an-aardvark not-an-aardvark deleted the repl-autocomplete-folder-extensions branch August 13, 2017 08:12
addaleax pushed a commit that referenced this pull request Aug 13, 2017
@not-an-aardvark @addaleax
repl: include folder extensions in autocomplete
1c00875 
When autocompleting `require` calls, the repl strips .js file extensions
from results. However, stripping an extension from a directory results
in an error. Update the autocompletion logic to avoid stripping
extensions from directories.

PR-URL: #14727
Fixes: #14726
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
@addaleax addaleax mentioned this pull request Aug 13, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@aqrln aqrln aqrln approved these changes

@hiroppy hiroppy hiroppy approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

require() autocompletion in repl incorrectly removes file extensions from folders
10 participants
@not-an-aardvark @Trott @benjamingr @TimothyGu @cjihrig @tniessen @hiroppy @aqrln @BridgeAR @MylesBorins