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

repl: don’t write to input stream in editor mode by addaleax · Pull Request #9207 · nodejs/node · GitHub

/ node Public

repl: don’t write to input stream in editor mode - #9207

Closed
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:fix-repl-write-editor-mode
ConversationCommits (3)ChecksFiles changed
Closed

repl: don’t write to input stream in editor mode#9207
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:fix-repl-write-editor-mode

Conversation

addaleax commented Oct 20, 2016

Copy link
Copy Markdown
Member
Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

Description of change
  • In .editor mode, repl.write() would have crashed when the key argument was not present, because the overwritten _ttyWrite of REPLs doesn’t check for the absence of a second argument like readline.write() does.
    Since the docs indicate that the argument is optional, add a check paralleling the one in readline.write().
  • Instead of writing to the REPL’s input stream for the alignment
    spaces in .editor mode, let readline handle the spaces
    properly (echoing them using _ttyWrite and adding them to the
    current line buffer).

Fixes: #9189

repl: make key of repl.write() optional always
37c9d87 
In `.editor` mode, `repl.write()` would have crashed when the
`key` argument was not present, because the overwritten
`_ttyWrite` of REPLs doesn’t check for the absence of a second
argument like `readline.write()` does.

Since the docs indicate that the argument is optional, add
a check paralleling the one in `readline.write()`.
nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Oct 20, 2016

addaleax commented Oct 20, 2016

Copy link
Copy Markdown
Member Author

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

/cc @princejwesley

princejwesley reviewed Oct 20, 2016
View reviewed changes
Comment thread test/parallel/test-repl-.editor.js Outdated

princejwesley Oct 20, 2016

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

@addaleax quoted terminal code is sufficient right?
const terminalCodeRegex = /\u001b\[1G\u001b\[0J> \u001b\[3G/g

addaleax Oct 20, 2016

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

@princejwesley Yeah, I’ve updated it to use a regex… I’m not particularly a fan of duplicating strings that are hard to scan, but if you prefer a plain regex literal over what I’ve just pushed, I can go with that too

princejwesley Oct 20, 2016

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

@addaleax 👍 Just to convey the term 'quoted' correctly, added code snippet with [ escaped manually 😄

addaleax Oct 20, 2016

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

@princejwesley ok, seems like we’re on the same page then 😄

[squash] use regex for control code + prompt stripping
d197827
repl: don’t write to input stream in editor mode
979528d 
Instead of writing to the REPL’s input stream for the alignment
spaces in `.editor` mode, let `readline` handle the spaces
properly (echoing them using `_ttyWrite` and adding them to the
current line buffer).

Fixes: nodejs#9189
addaleax force-pushed the fix-repl-write-editor-mode branch from f536ac6 to 979528d Compare October 20, 2016 15:28
princejwesley approved these changes Oct 20, 2016
View reviewed changes

princejwesley 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

addaleax commented Oct 26, 2016

Copy link
Copy Markdown
Member Author

Landed in 8b6fd43 and 3dfc127

addaleax added a commit that referenced this pull request Oct 26, 2016
repl: make key of repl.write() optional always
8b6fd43 
In `.editor` mode, `repl.write()` would have crashed when the
`key` argument was not present, because the overwritten
`_ttyWrite` of REPLs doesn’t check for the absence of a second
argument like `readline.write()` does.

Since the docs indicate that the argument is optional, add
a check paralleling the one in `readline.write()`.

PR-URL: #9207
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Oct 26, 2016
repl: don’t write to input stream in editor mode
3dfc127 
Instead of writing to the REPL’s input stream for the alignment
spaces in `.editor` mode, let `readline` handle the spaces
properly (echoing them using `_ttyWrite` and adding them to the
current line buffer).

Fixes: #9189
PR-URL: #9207
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax closed this Oct 26, 2016
addaleax deleted the fix-repl-write-editor-mode branch October 26, 2016 19:49
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
repl: make key of repl.write() optional always
1c59cef 
In `.editor` mode, `repl.write()` would have crashed when the
`key` argument was not present, because the overwritten
`_ttyWrite` of REPLs doesn’t check for the absence of a second
argument like `readline.write()` does.

Since the docs indicate that the argument is optional, add
a check paralleling the one in `readline.write()`.

PR-URL: #9207
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
repl: don’t write to input stream in editor mode
0b9d80a 
Instead of writing to the REPL’s input stream for the alignment
spaces in `.editor` mode, let `readline` handle the spaces
properly (echoing them using `_ttyWrite` and adding them to the
current line buffer).

Fixes: #9189
PR-URL: #9207
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas mentioned this pull request Nov 3, 2016
Merged

MylesBorins commented Nov 22, 2016

Copy link
Copy Markdown
Contributor

@addaleax lts?

addaleax commented Nov 22, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd v6.x only, but yes

MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
repl: make key of repl.write() optional always
ed3de08 
In `.editor` mode, `repl.write()` would have crashed when the
`key` argument was not present, because the overwritten
`_ttyWrite` of REPLs doesn’t check for the absence of a second
argument like `readline.write()` does.

Since the docs indicate that the argument is optional, add
a check paralleling the one in `readline.write()`.

PR-URL: #9207
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
repl: don’t write to input stream in editor mode
58b60fc 
Instead of writing to the REPL’s input stream for the alignment
spaces in `.editor` mode, let `readline` handle the spaces
properly (echoing them using `_ttyWrite` and adding them to the
current line buffer).

Fixes: #9189
PR-URL: #9207
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Nov 22, 2016
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
+1 more reviewer
princejwesley princejwesley 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.

5 participants

Back | FazBrowse Home | New Git URL