FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

repl: better handling of recoverable errors by princejwesley · Pull Request #18915 · nodejs/node · GitHub

/ node Public

repl: better handling of recoverable errors - #18915

Closed
princejwesley wants to merge 1 commit into
nodejs:masterfrom
princejwesley:repl.cont
ConversationCommits (1)ChecksFiles changed
Closed

repl: better handling of recoverable errors#18915
princejwesley wants to merge 1 commit into
nodejs:masterfrom
princejwesley:repl.cont

Conversation

princejwesley commented Feb 21, 2018

Copy link
Copy Markdown
Contributor

Better handling of recoverable errors in REPL module

Below syntax errors are handled without force .break/clear

  • Unexpected Token (prefix errors)
  • missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Feb 21, 2018
Leko approved these changes Feb 24, 2018
View reviewed changes

Leko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Leko commented Feb 24, 2018

Copy link
Copy Markdown
Contributor

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

Leko commented Feb 25, 2018

Copy link
Copy Markdown
Contributor

CI failed but I think those errors are not related to this PR.

not ok 712 parallel/test-http-pause
  ---
  duration_ms: 0.213
  severity: fail
  stack: |-
  ...

https://ci.nodejs.org/job/node-test-commit-linux/nodes=debian8-64/16655/console

not ok 2002 sequential/test-fs-readfile-tostring-fail
  ---
  duration_ms: 8.50
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 9)
  ...

https://ci.nodejs.org/job/node-test-commit-linux/nodes=centos7-64/16655/console

Leko commented Feb 26, 2018

Copy link
Copy Markdown
Contributor

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

0joshuaolson1 commented Feb 27, 2018 •
edited
Loading

Copy link
Copy Markdown

Is this the right place to add other syntax errors that the repl doesn't handle correctly? I can't find an issue for it.

0joshuaolson1 mentioned this pull request Feb 27, 2018
Closed
3 tasks
BridgeAR approved these changes Mar 2, 2018
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. Just a nit and a question.

Comment thread lib/repl.js Outdated

BridgeAR Mar 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: please remove the else. This is a style that is normally not used here.

princejwesley Mar 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

rebasing and amending to same commit

Comment thread test/parallel/test-repl.js Outdated

BridgeAR Mar 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I guess recovering from e.g.

'`abc ${ test'

is not possible anymore?

princejwesley Mar 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, only some forms are not possible. For instance, below one is possible.

`abc ${
   test}`

Its mostly consistent with chrome console behaviour except for couple of cases.

  1. we handle 'missing ) after argument list' error better than chrome console.
  2. we allow repl commands in multiline mode which should otherwise be treated as properties/functions.
    princejwesley@c1796b8 (wip)
> (function() {
... x = { help: () => 'help received' };
... return x
...         .help ();    <-- no repl command parsing in multiline mode
.break    Sometimes you get stuck, this gets you out
.clear    Alias for .break
.editor   Enter editor mode
.exit     Exit the repl
.help     Print this help message
.load     Load JS from a file into the REPL session
.save     Save all evaluated commands in this REPL session to a file
...

(I'll give PR over the weekend)

BridgeAR commented Mar 2, 2018

Copy link
Copy Markdown
Member

@0joshuaolson1 please open a new issue in case you think the repl should recover from errors that you ran into.

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 2, 2018
repl: better handling of recoverable errors
b9f6453 
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

BridgeAR commented Mar 6, 2018

Copy link
Copy Markdown
Member

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

BridgeAR commented Mar 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Landed in ebfa8b1 🎉

BridgeAR closed this Mar 11, 2018
BridgeAR pushed a commit that referenced this pull request Mar 11, 2018
repl: better handling of recoverable errors
ebfa8b1 
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

PR-URL: #18915
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Mar 17, 2018
repl: better handling of recoverable errors
47f4a33 
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

PR-URL: #18915
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos mentioned this pull request Mar 18, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
repl: better handling of recoverable errors
3e6858e 
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

PR-URL: #18915
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
repl: better handling of recoverable errors
93b9dc0 
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

PR-URL: nodejs#18915
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
repl: better handling of recoverable errors
8c206d7 
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

PR-URL: nodejs#18915
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
repl: better handling of recoverable errors
a6fca75 
Below syntax errors are handled without force .break/clear
  - Unexpected Token (prefix errors)
  - missing ) after argument list

In the multiline expression, recoverable errors are truly
recoverable, otherwise syntax error will be thrown.

Backport-PR-URL: #22380
PR-URL: #18915
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins mentioned this pull request Sep 6, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
BridgeAR BridgeAR approved these changes
+2 more reviewers
daynin daynin approved these changes
Leko Leko approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

8 participants

Back | FazBrowse Home | New Git URL