add escapeCodeTimeout as an option to createInterface by raoofha · Pull Request #19780 · nodejs/node

raoofha · 2018-04-03T19:39:45Z

500ms for application like readline-vim is very annoying. this PR allow the developer to control the timeout.

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Trott · 2018-04-03T20:36:05Z

Hi, @raoofha and welcome! Thanks for the pull request!

Some immediate thoughts:

If we're going to export this, we'll need to add documentation for it. Totally OK if you're not comfortable writing the final text, but a draft of what you think should go in the docs would be very helpful.
Since this adds functionality, there should be a test for it. At a minimum, a test file should load readline and confirm that the property is exported, confirm that setting it to a few invalid values has the expected affect (which should be documented--ignore invalid value? throw? coerce to something valid? something else?), and (if possible) confirm that setting it to something very low and/or very high has the desired affect.
If we're exporting it, then it's no longer a constant and we probably want to change the name from ESCAPE_CODE_TIMEOUT to something more like escapeCodeTimeout.

/ping @thlorenz

addaleax · 2018-04-03T20:39:44Z

Would it make sense to do this on a per-instance basis, rather than for each Node.js process as a whole?

jasnell · 2018-04-03T20:54:28Z

lib/readline.js

If we're going to do this, passing escapeCodeTimeout as an option to readline.createInterface() would seem to be the better approach.

raoofha · 2018-04-04T06:49:30Z

I add doc ( copyed from here ) and manually test it on my machine. I don't know how to write test for it, if you can tell me how maybe I can do it. thanks

Trott · 2018-04-04T12:23:58Z

Sorry @raoofha, I should have included this link with my comment! https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md

Hope it helps. (And if it doesn't, maybe you can give some feedback and we can try to improve it!)

raoofha · 2018-04-04T16:18:43Z

thanks @Trott for your comments. I added a simple test and waiting for your review.

Trott · 2018-04-04T16:59:07Z

I rebased to get rid of the conflict, and also reworded the documentation. (Hope that's OK!)

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

Trott · 2018-04-04T19:18:44Z

CI failures are unrelated. Re-running relevant jobs.

Re-run FreeBSD: https://ci.nodejs.org/job/node-test-commit-freebsd/16720/
Re-run Linux: https://ci.nodejs.org/job/node-test-commit-linux/17640/

BridgeAR · 2018-04-09T11:12:06Z

lib/readline.js

Suggestion: the check could be moved in the upper if.

BridgeAR · 2018-04-09T11:13:41Z

lib/readline.js

Suggestion: just write:

timeoutId = setTimeout(escapeCodeTimeoutFn, iface ? iface.escapeCodeTimeout : ESCAPE_CODE_TIMEOUT);

BridgeAR · 2018-04-09T11:15:25Z

doc/api/readline.md

It is unclear in what unit the time is measured here. It should say something like The duration readline will wait for a character when reading an ambiguous key sequence in milliseconds (...).

I personally would also only accept integers but that is just me.

BridgeAR · 2018-04-28T15:04:51Z

test/parallel/test-readline-interface-escapecodetimeout.js

These tests are doing all the same thing and are just duplicated. It would be best to write:

[ null, {}, NaN, '50' ].forEach((invalidInput) => { const fi = new FakeInput(); const rli = new readline.Interface({ input: fi, output: fi, escapeCodeTimeout: invalidInput }); assert.strictEqual(rli.escapeCodeTimeout, ESCAPE_CODE_TIMEOUT); rli.close(); });

cjihrig

Left a few comments. Mostly trying to make the diff less noisy.

cjihrig · 2018-05-05T18:38:33Z

doc/api/readline.md

The parentheses should probably go next to "ambiguous key sequence"

cjihrig · 2018-05-05T18:43:21Z

lib/readline.js

Perhaps Number.isFinite() should be used here. It's also probably better to throw, rather than silently use a different value.

cjihrig · 2018-05-05T18:45:54Z

lib/readline.js

Could you remove this new blank line.

cjihrig · 2018-05-05T18:47:25Z

lib/readline.js

escapeCodeTimeout isn't used between here and line 101. I don't think you need to create an intermediate variable for it.

cjihrig · 2018-05-05T18:48:12Z

lib/readline.js

And if you don't introduce a new intermediate variable, you don't need to rename this.

BridgeAR · 2018-05-15T15:38:21Z

@raoofha if you have some time, would you be so kind and have a look at the comments? :-)

raoofha · 2018-05-15T17:07:28Z

@BridgeAR sorry, I will

raoofha · 2018-05-15T19:01:37Z

@cjihrig @BridgeAR thanks for your reviews please look at the last commit, and sorry for the delay

BridgeAR · 2018-05-15T20:20:24Z

lib/readline.js

This should be moved into the added upper if statement, since it is only necessary to test to possible provided option. If non was passed through, we already know that the value is finite.

raoofha · 2018-05-16T05:11:36Z

@BridgeAR please take a look again

raoofha · 2018-07-12T06:23:10Z

hi @BridgeAR wondering when this is going to merge, or is there something that I have to do first?

maclover7 · 2018-08-11T20:36:08Z

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

@BridgeAR I believe your comments have been addressed, PTAL

jasnell · 2018-09-10T18:10:23Z

Ping @BridgeAR

lundibundi · 2018-10-15T20:32:42Z

~~CI: https://ci.nodejs.org/job/node-test-pull-request/17876/~~
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/17891/

Changes LGTM but I'm not familiar with realine, hence no green mark.

ping @BridgeAR.

BridgeAR

LGTM

BridgeAR · 2018-10-16T07:52:42Z

Just as note: this did not have to wait on me. I did not block anything.

refack · 2018-10-16T13:50:21Z

Resume: https://ci.nodejs.org/job/node-test-pull-request/17905/

escapeCodeTimeout option in createInterface default to ESCAPE_CODE_TIMEOUT = 500 when set to a NaN or negative number or a value other than a number set back to default value

@BridgeAR

as suggested by @BridgeAR

refack · 2018-10-16T14:49:07Z

Had to rebase in order to get a proper CI: https://ci.nodejs.org/job/node-test-pull-request/17910/

Trott · 2018-10-18T04:48:02Z

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/17960/

PR-URL: #19780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

jasnell · 2018-10-25T23:52:21Z

Landed in c829202

PR-URL: #19780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: #19780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Signed-off-by: Beth Griggs <[email protected]>

nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Apr 3, 2018

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 3, 2018

jasnell requested changes Apr 3, 2018

View reviewed changes

raoofha changed the title ~~export ESCAPE_CODE_TIMEOUT~~ add escapeCodeTimeout as an option to createInterface Apr 4, 2018

Trott force-pushed the master branch from 4d288d9 to 6176779 Compare April 4, 2018 16:58

jasnell approved these changes Apr 4, 2018

View reviewed changes

BridgeAR reviewed Apr 9, 2018

View reviewed changes

BridgeAR reviewed Apr 28, 2018

View reviewed changes

cjihrig reviewed May 5, 2018

View reviewed changes

BridgeAR reviewed May 15, 2018

View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

BridgeAR approved these changes Oct 16, 2018

View reviewed changes

BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed stalled Issues and PRs that are stalled. labels Oct 16, 2018

raoofha and others added 6 commits October 16, 2018 10:47

export ESCAPE_CODE_TIMEOUT

991174e

lib: add escapeCodeTimeout as an option to createInterface

a58d9db

escapeCodeTimeout option in createInterface default to ESCAPE_CODE_TIMEOUT = 500 when set to a NaN or negative number or a value other than a number set back to default value

test: add simple test to ensures escapeCodeTimeout option set correctly

0586244

lib: rewrite the same code for escapeCodeTimeout option

b0b8ba5

as suggested by @BridgeAR

lib: rewrite the same code again :)

fb1ce38

lib: rewrite the same code for the third time

5a25fd5

refack force-pushed the master branch from a83c9d8 to 5a25fd5 Compare October 16, 2018 14:48

jasnell pushed a commit that referenced this pull request Oct 25, 2018

lib: add escapeCodeTimeout as an option to createInterface

c829202

PR-URL: #19780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

jasnell closed this Oct 25, 2018

targos pushed a commit that referenced this pull request Oct 26, 2018

lib: add escapeCodeTimeout as an option to createInterface

2e91771

PR-URL: #19780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

targos mentioned this pull request Oct 27, 2018

Release proposal: v11.1.0 #23922

Merged

targos pushed a commit that referenced this pull request Nov 1, 2018

lib: add escapeCodeTimeout as an option to createInterface

1c5ffb3

PR-URL: #19780 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

BethGriggs mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

Uh oh!

Conversation

raoofha commented Apr 3, 2018

Uh oh!

Trott commented Apr 3, 2018

Uh oh!

addaleax commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raoofha commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Apr 4, 2018

Uh oh!

raoofha commented Apr 4, 2018

Uh oh!

Trott commented Apr 4, 2018

Uh oh!

Trott commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented May 15, 2018

Uh oh!

raoofha commented May 15, 2018

Uh oh!

raoofha commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raoofha commented May 16, 2018

Uh oh!

raoofha commented Jul 12, 2018

Uh oh!

maclover7 commented Aug 11, 2018

Uh oh!

jasnell commented Sep 10, 2018

Uh oh!

lundibundi commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Oct 16, 2018

Uh oh!

refack commented Oct 16, 2018

Uh oh!

refack commented Oct 16, 2018

Uh oh!

Trott commented Oct 18, 2018

Uh oh!

jasnell commented Oct 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

raoofha commented Apr 4, 2018 •

edited

Loading

lundibundi commented Oct 15, 2018 •

edited

Loading