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

lib: avoid REPL exit on completion error #3358

Merged
merged 1 commit into from
Oct 21, 2015
Merged

lib: avoid REPL exit on completion error #3358

merged 1 commit into from
Oct 21, 2015

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 14, 2015

If a tab completion is attempted on an undefined reference inside of a
function, the REPL was exiting without reporting an error or anything
else. This change results in the REPL reporting the ReferenceError and
continuing.

Fixes: #3346

@Trott Trott mentioned this pull request Oct 14, 2015
Closed
@Trott
Copy link
Member Author

Trott commented Oct 14, 2015

CI: https://ci.nodejs.org/job/node-test-commit/834/

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Oct 14, 2015
@Trott
Copy link
Member Author

Trott commented Oct 14, 2015

Fixed borkage. CI take two: https://ci.nodejs.org/job/node-test-commit/836/

@Trott
Copy link
Member Author

Trott commented Oct 14, 2015

Removed a line, so one more CI: https://ci.nodejs.org/job/node-test-commit/837/

@Trott
Copy link
Member Author

Trott commented Oct 14, 2015

More tweaks! I should go to sleep. Last CI for a while I hope: https://ci.nodejs.org/job/node-test-commit/838/

@bnoordhuis
Copy link
Member

LGTM. The addition of the .top property makes it technically a semver-minor, I think?

@Trott
Copy link
Member Author

Trott commented Oct 14, 2015

@bnoordhuis Since it's intended for internal use only, perhaps I can hide it a bit by making the property key a Symbol rather than the string top. Or if that's overkill, maybe I can at least prefix it with _.

@bnoordhuis
Copy link
Member

I'd go with a symbol.

@Trott
Copy link
Member Author

Trott commented Oct 14, 2015

@bnoordhuis I did a Symbol implementation but then went a step further and hid the properties entirely using a WeakMap. PTAL

@Trott
Copy link
Member Author

Trott commented Oct 14, 2015

CI: https://ci.nodejs.org/job/node-test-commit/845/

@Trott
Copy link
Member Author

Trott commented Oct 20, 2015

Bump. I think this is good for a merge... /cc @bnoordhuis

@jasnell
Copy link
Member

jasnell commented Oct 21, 2015

LGTM... err. I mean, WTIC /cc @Trott

@jasnell
Copy link
Member

jasnell commented Oct 21, 2015

@Trott ... can I ask you to squash the commits and I'll get it landed.

@Trott
lib: avoid REPL exit on completion error
b354be7 
If a tab completion is attempted on an undefined reference inside of a
function, the REPL was exiting without reporting an error or anything
else. This change results in the REPL reporting the ReferenceError and
continuing.

Fixes: nodejs#3346
PR-URL: nodejs#3358
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott force-pushed the fix-issue-3346 branch 2 times, most recently from f8e494b to b354be7 Compare October 21, 2015 22:35
@Trott
Copy link
Member Author

Trott commented Oct 21, 2015

Rebased to master, squashed, running one last CI before landing on master: https://ci.nodejs.org/job/node-test-commit/909/

@Trott Trott merged commit b354be7 into nodejs:master Oct 21, 2015
@Trott
Copy link
Member Author

Trott commented Oct 21, 2015

Landed in b354be7

Trott added a commit that referenced this pull request Oct 22, 2015
@Trott @rvagg
lib: avoid REPL exit on completion error
97d0817 
If a tab completion is attempted on an undefined reference inside of a
function, the REPL was exiting without reporting an error or anything
else. This change results in the REPL reporting the ReferenceError and
continuing.

Fixes: #3346
PR-URL: #3358
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott added a commit that referenced this pull request Oct 26, 2015
@Trott @jasnell
lib: avoid REPL exit on completion error
1888f84 
If a tab completion is attempted on an undefined reference inside of a
function, the REPL was exiting without reporting an error or anything
else. This change results in the REPL reporting the ReferenceError and
continuing.

Fixes: #3346
PR-URL: #3358
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

Landed in v4.x-staging in 1888f84

This was referenced Oct 27, 2015
Merged
Closed
Trott added a commit that referenced this pull request Oct 29, 2015
@Trott @jasnell
lib: avoid REPL exit on completion error
f49c7c6 
If a tab completion is attempted on an undefined reference inside of a
function, the REPL was exiting without reporting an error or anything
else. This change results in the REPL reporting the ReferenceError and
continuing.

Fixes: #3346
PR-URL: #3358
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request Oct 29, 2015
Closed
@Fishrock123
Copy link
Contributor

@Trott just noticing this, could you please make sure to be more specific for the subsystem? i.e. repl instead of lib (Only use lib when it really affects like everything, or more than 2-3). :D

@Trott
Copy link
Member Author

Trott commented Nov 3, 2015

👍

@Trott Trott deleted the fix-issue-3346 branch January 13, 2022 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.
5 participants
@Trott @bnoordhuis @jasnell @Fishrock123 @mscdex