readline: refactor readline module #12755

jasnell · 2017-04-30T03:56:23Z

Refactor parts of the readline module

Move to more efficient module.exports = {} pattern
Restructure imports and other minor code cleanups
Move CSI (escape codes) into internal/readline for easier maintenance
Improve test coverage

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)

readline

Variety of code maintenance updates, cleanups

jasnell · 2017-04-30T03:57:32Z

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

refack · 2017-04-30T12:16:33Z

IMHO in order not to conflict with @12711 change the variable function to arrows

refack · 2017-04-30T12:19:17Z

P.S. freeBSD seems unrelated:

731	parallel/test-net-connect-local-error	
duration_ms	0.364
severity	fail
stack	
assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: undefined !== 12346 in Error: connect EADDRINUSE 127.0.0.1:12347 - Local (127.0.0.1:12346)
    at Socket.onError (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-net-connect-local-error.js:13:10)
    at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/common.js:461:15)
    at emitOne (events.js:115:13)
    at Socket.emit (events.js:210:7)
    at emitErrorNT (net.js:1302:8)
    at _combinedTickCallback (internal/process/next_tick.js:81:11)
    at process._tickCallback (internal/process/next_tick.js:105:9)

addaleax

Mostly LGTM, got one tiny correction though ;)

addaleax · 2017-04-30T18:49:10Z

lib/internal/readline.js

@@ -7,23 +7,38 @@
 const ansi =
  /[\u001b\u009b][[()#;?]*(?:[0-9]{1,4}(?:;[0-9]{0,4})*)?[0-9A-ORZcf-nqry=><]/g;

+const kCSI = '\x1b';


The CSI would be \x1b[ (2 chars) or \x9b, this is just an escape character, so it would probably be a good idea to rename this to kEscape, or use \x1b[ here

Yeah, good point :-)

Moves escape codes into internal/readline for easier management.

jasnell · 2017-05-01T18:04:49Z

@addaleax ... updated!

addaleax · 2017-05-03T17:32:28Z

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

addaleax · 2017-05-07T20:59:10Z

Landed in a398516...d37f27a

PR-URL: #12755 Reviewed-By: Anna Henningsen <[email protected]>

Variety of code maintenance updates, cleanups PR-URL: #12755 Reviewed-By: Anna Henningsen <[email protected]>

Moves escape codes into internal/readline for easier management. PR-URL: #12755 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #12755 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs#12755 Reviewed-By: Anna Henningsen <[email protected]>

Variety of code maintenance updates, cleanups PR-URL: nodejs#12755 Reviewed-By: Anna Henningsen <[email protected]>

Moves escape codes into internal/readline for easier management. PR-URL: nodejs#12755 Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: nodejs#12755 Reviewed-By: Anna Henningsen <[email protected]>

gibfahn · 2017-06-20T22:40:11Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Very possibly not worth backporting, your call @jasnell .

jasnell added 3 commits April 29, 2017 13:33

readline: use module.exports = {}

e5e57b2

readline: use module.exports = {} on internal/readline

f5f254b

readline: multiple code cleanups

88eb5cd

Variety of code maintenance updates, cleanups

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

jasnell changed the title ~~Refactor readline~~ readline: refactor readline module Apr 30, 2017

TimothyGu added the refactor to ES6+ label Apr 30, 2017

addaleax requested changes Apr 30, 2017

View reviewed changes

jasnell added 2 commits May 1, 2017 10:49

readline: move escape codes into internal/readline

291a464

Moves escape codes into internal/readline for easier management.

test: expand test coverage of readline

a5521a6

jasnell force-pushed the refactor-readline branch from 58cdaec to a5521a6 Compare May 1, 2017 18:04

addaleax approved these changes May 1, 2017

View reviewed changes

addaleax closed this May 7, 2017

addaleax pushed a commit that referenced this pull request May 7, 2017

readline: use module.exports = {}

9318f82

PR-URL: #12755 Reviewed-By: Anna Henningsen <[email protected]>

addaleax pushed a commit that referenced this pull request May 7, 2017

readline: use module.exports = {} on internal/readline

392a898

PR-URL: #12755 Reviewed-By: Anna Henningsen <[email protected]>

addaleax pushed a commit that referenced this pull request May 7, 2017

readline: multiple code cleanups

4ac7a68

Variety of code maintenance updates, cleanups PR-URL: #12755 Reviewed-By: Anna Henningsen <[email protected]>

addaleax pushed a commit that referenced this pull request May 7, 2017

readline: move escape codes into internal/readline

4c070d4

Moves escape codes into internal/readline for easier management. PR-URL: #12755 Reviewed-By: Anna Henningsen <[email protected]>

addaleax pushed a commit that referenced this pull request May 7, 2017

test: expand test coverage of readline

d37f27a

PR-URL: #12755 Reviewed-By: Anna Henningsen <[email protected]>

YurySolovyov mentioned this pull request May 12, 2017

Migrate squirrel-update to JS atom/atom#14385

Closed

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017

readline: use module.exports = {}

5f25bdf

PR-URL: nodejs#12755 Reviewed-By: Anna Henningsen <[email protected]>

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017

readline: use module.exports = {} on internal/readline

5ec24d8

PR-URL: nodejs#12755 Reviewed-By: Anna Henningsen <[email protected]>

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017

readline: multiple code cleanups

1a6c82c

Variety of code maintenance updates, cleanups PR-URL: nodejs#12755 Reviewed-By: Anna Henningsen <[email protected]>

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017

readline: move escape codes into internal/readline

6d50843

Moves escape codes into internal/readline for easier management. PR-URL: nodejs#12755 Reviewed-By: Anna Henningsen <[email protected]>

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017

test: expand test coverage of readline

7a034c5

PR-URL: nodejs#12755 Reviewed-By: Anna Henningsen <[email protected]>

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the backport-requested-v6.x label Jun 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

readline: refactor readline module #12755

readline: refactor readline module #12755

jasnell commented Apr 30, 2017

jasnell commented Apr 30, 2017

refack commented Apr 30, 2017

refack commented Apr 30, 2017

addaleax left a comment

addaleax Apr 30, 2017

jasnell May 1, 2017

jasnell commented May 1, 2017

addaleax commented May 3, 2017

addaleax commented May 7, 2017

gibfahn commented Jun 20, 2017

readline: refactor readline module #12755

readline: refactor readline module #12755

Conversation

jasnell commented Apr 30, 2017

Checklist

Affected core subsystem(s)

jasnell commented Apr 30, 2017

refack commented Apr 30, 2017

refack commented Apr 30, 2017

addaleax left a comment

Choose a reason for hiding this comment

addaleax Apr 30, 2017

Choose a reason for hiding this comment

jasnell May 1, 2017

Choose a reason for hiding this comment

jasnell commented May 1, 2017

addaleax commented May 3, 2017

addaleax commented May 7, 2017

gibfahn commented Jun 20, 2017