Skip to content

repl: include folder extensions in autocomplete #14727

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
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

@not-an-aardvark not-an-aardvark commented Aug 10, 2017

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

not-an-aardvark commented Aug 10, 2017

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
Copy link
Contributor Author

@not-an-aardvark not-an-aardvark Aug 10, 2017

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

@BridgeAR BridgeAR Aug 10, 2017

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
Copy link
Member

@BridgeAR BridgeAR Aug 10, 2017

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
Copy link
Member

@BridgeAR BridgeAR Aug 10, 2017

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

not-an-aardvark commented Aug 11, 2017

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
Copy link
Member

@BridgeAR BridgeAR Aug 11, 2017

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

not-an-aardvark commented Aug 11, 2017

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 <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Aug 13, 2017

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 <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@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

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+3 more reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@hiroppy hiroppy hiroppy approved these changes

@aqrln aqrln aqrln approved these changes

Reviewers whose approvals may not affect merge requirements

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